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

Tests: use classes instead of paths in settings #4817

Merged
merged 2 commits into from Oct 2, 2020

Conversation

elacuesta
Copy link
Member

Simplify tests after #3873

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #4817 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4817      +/-   ##
==========================================
+ Coverage   87.52%   87.54%   +0.02%     
==========================================
  Files         160      160              
  Lines        9761     9761              
  Branches     1440     1440              
==========================================
+ Hits         8543     8545       +2     
+ Misses        955      954       -1     
+ Partials      263      262       -1     
Impacted Files Coverage Δ
scrapy/core/downloader/__init__.py 92.48% <0.00%> (+1.50%) ⬆️

Copy link
Member

@wRAR wRAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this so much more!

@@ -56,9 +58,9 @@ class ProcessSpiderInputSpiderWithoutErrback(Spider):
custom_settings = {
'SPIDER_MIDDLEWARES': {
# spider
__name__ + '.LogExceptionMiddleware': 10,
__name__ + '.FailProcessSpiderInputMiddleware': 8,
__name__ + '.LogExceptionMiddleware': 6,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like it currently uses only one of these entries? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the behaviour makes sense. The failing middleware fails in process_spider_input and does not define a process_spider_exception method, so LogExceptionMiddleware.process_spider_exception (the method that logs the exception) is placed at the beginning of the chain as expected. In fact, we only need one of the occurrences for the test case, I'm removing the duplicate one.
Nice catch, thanks for noticing!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For reference)
current master (5a38639):

  Enabled spider middlewares:
['tests.test_spidermiddleware_output_chain.LogExceptionMiddleware',
 'tests.test_spidermiddleware_output_chain.FailProcessSpiderInputMiddleware',
 'scrapy.spidermiddlewares.httperror.HttpErrorMiddleware',
 'scrapy.spidermiddlewares.offsite.OffsiteMiddleware',
 'scrapy.spidermiddlewares.referer.RefererMiddleware',
 'scrapy.spidermiddlewares.urllength.UrlLengthMiddleware',
 'scrapy.spidermiddlewares.depth.DepthMiddleware']

current test-classes-settings (62394c2):

   Enabled spider middlewares:
[<class 'tests.test_spidermiddleware_output_chain.LogExceptionMiddleware'>,
 <class 'tests.test_spidermiddleware_output_chain.FailProcessSpiderInputMiddleware'>,
 'scrapy.spidermiddlewares.httperror.HttpErrorMiddleware',
 'scrapy.spidermiddlewares.offsite.OffsiteMiddleware',
 'scrapy.spidermiddlewares.referer.RefererMiddleware',
 'scrapy.spidermiddlewares.urllength.UrlLengthMiddleware',
 'scrapy.spidermiddlewares.depth.DepthMiddleware']

@elacuesta elacuesta merged commit 797a669 into scrapy:master Oct 2, 2020
@elacuesta elacuesta deleted the test-classes-settings branch October 2, 2020 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants