Skip to content
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

Make DNS retry test compatible with Twisted 17+ #2528

Merged
merged 3 commits into from Feb 2, 2017

Conversation

@redapple
Copy link
Contributor

@redapple redapple commented Feb 2, 2017

Follow up on #2462 and #2521

crawler = self.runner.create_crawler(SimpleSpider)
with LogCapture() as l:
# try to fetch the homepage of a non-existent domain
yield crawler.crawl("http://dns.resolution.invalid/")

This comment has been minimized.

@kmike

kmike Feb 2, 2017
Member

In the original PR (877e345) @dangra mentioned that localhost666 can be resolvable under certain circumstances. Does it apply for .invalid domains as well?

This comment has been minimized.

@redapple

redapple Feb 2, 2017
Author Contributor

According to @glyph in #2521 (comment) and the referenced RFC2606 , .invalid is a reserved TLD, so I hope it's fine to use here.

This comment has been minimized.

@dangra

dangra Feb 2, 2017
Member

it is fine as long as you add a final .. .invalid only ensures that any domainname under that tld won't ever exists.

This comment has been minimized.

@dangra

dangra Feb 2, 2017
Member

to be more precise, the final url should be http://dns.resolution.invalid./.

This comment has been minimized.

@glyph

glyph Feb 2, 2017

A trailing . in a domain name - or the authority section of a URL, for that matter - is perfectly valid; it's used to indicate top-level termination rather than using an implicit search domain. If the crawler isn't fetching those it's a bug somewhere in the HTTP stack.

This comment has been minimized.

@glyph

glyph Feb 2, 2017

Oops, I think I misunderstood @dangra's comment. I can see why you'd want to use the trailing dot there :).

This comment has been minimized.

@redapple

redapple Feb 2, 2017
Author Contributor

alright @dangra , @kmike , @glyph , done. Thanks the input.

@codecov-io
Copy link

@codecov-io codecov-io commented Feb 2, 2017

Codecov Report

Merging #2528 into master will not impact coverage.

@@           Coverage Diff           @@
##           master    #2528   +/-   ##
=======================================
  Coverage   83.49%   83.49%           
=======================================
  Files         161      161           
  Lines        8787     8787           
  Branches     1289     1289           
=======================================
  Hits         7337     7337           
  Misses       1203     1203           
  Partials      247      247

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c305c46...02e1d2b. Read the comment docs.

redapple added 2 commits Feb 2, 2017
@kmike
Copy link
Member

@kmike kmike commented Feb 2, 2017

Thanks @redapple, @dangra and @glyph!

@kmike kmike merged commit 814ce37 into scrapy:master Feb 2, 2017
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing c305c46...02e1d2b
Details
codecov/project 83.49% remains the same compared to c305c46
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@redapple redapple deleted the redapple:dns-retry-test branch Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants