-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tests #3
Conversation
jage
commented
Feb 20, 2014
- Add basic tests so I can refactor
- Refactor
- Add tests for weird URLs
- Fix red tests
- Add profiling tests
- Optimize
- 💵
- Bump Gem version
Using minitest with some extras: * Turn for more informative run output * Shoulda for context and matchers Turn: https://github.com/turn-project/turn Shoulda: https://github.com/thoughtbot/shoulda
Broken URLs found during work with Zambezi
end | ||
|
||
should "handle URL with reference to another URL in it" do | ||
url = "http://news.google.com/news/url?sa=t&fd=R&usg=AFQjCNGc4A_sfGS6fMMqggiK_8h6yk2miw&url=http:%20%20%20//fansided.com/2013/08/02/nike-decides-to-drop-milwaukee-brewers-ryan-braun" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this URL be supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause some library didn't like it (it's now working).
I don't know the RFC from my head, some strings might not be real URLs.
I agree this should be supported, if Chrome supports it it should work (we should snatch their tests!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works in curl too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle URL with reference to another URL in it
Hmm, is the "another URL" valid?
$ curl -v http:%20%20%20//fansided.com/2013/08/02/nike-decides-to-drop-milwaukee-brewers-ryan-braun
* Adding handle: conn: 0x7fbd82007a00
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fbd82007a00) send_pipe: 1, recv_pipe: 0
* Could not resolve host: http
* Closing connection 0
curl: (6) Could not resolve host: http
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't think it matters that it's a URL with another URL in it, parameters can contain whatever, don't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. I think the original site changed the "internal URL", replaced some stuff.
Chrome rewrites the URL to: http:+++//..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'm not sure the test name is that could, but I couldn't figure out a better one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. I think the original site changed the "internal URL", replaced some stuff.
Or not, Google says "The previous page is sending you to an invalid url".
shoulda includes shoulda-context and shoulda-matchers, we’re not using the matchar at this moment, so no need to pull it in (since it introduces lots of development dependencies).
PostRank::URI couldn’t handle umlauts. We will lose the feature to detect urls without protocol “twingly.com”, but we don’t see the need for this feature. On the plus side, lots of runtime dependencies are removed (nokogiri!).
assert_equal "http://www.twingly.com/", result | ||
end | ||
|
||
should "not be able to normalize url without protocol" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this so we don't add this feature by mistake in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart
With Postrank::URI
Without Postrank::URI
|
Much improve, so amaze! |
Inspiration from elasticsearch-transport tests: https://github.com/elasticsearch/elasticsearch-ruby/blob/6f83143b8e6409a 2eaf451a4dabf2c64f25ade31/elasticsearch-transport/test/profile/client_be nchmark_test.rb
I say I'm done! |
Should not exist in gems
In 19d28c6 when I removed Postrank::URI, I removed the feature that detected URLs without protocol. This commits enables tests for it again.
Enabled the behavior removed in 19d28c6 This uses PublicSuffix and Addressable instead of Postrank::URI though. Why? Postrank::URI was very slow, this is also slow, but not quite as slow.
Ok, we had some discussion about the changed behavior in this gem. I've added the old features again, but with new code. I'm using PublicSuffix instead of PostRank::URI. Since I'm verifying the domains, this is pretty slow. Not quite as slow as Postrank::URI though.
|
assert_equal [url], @normalizer.normalize(url) | ||
end | ||
|
||
should "should not blow up when there's no URL in the text" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One "should" too much?
From TestUrlHelper
FAIL (0:00:00.066) test_site_url_without_scheme
Expected: "//www.asos.com/"
Actual: "//www.asos.com"
@ /Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:200:in `assert'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:240:in `assert_equal'
test/unit/url_helper_test.rb:10:in `test_site_url_without_scheme'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:1301:in `run'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:867:in `_run_anything'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:1060:in `run_tests'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:1047:in `block in _run'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:1046:in `each'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:1046:in `_run'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:1035:in `run'
/Users/dentarg/.rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/minitest/unit.rb:789:in `block in autorun' |
Ok, I'll look into it. |
Insert / if no path exist.
So, what's next? Start using it and see where it breaks? |
I think so. Hopefully the tests in Zambezi and Stanley will pick any problems up :) |