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

Allow to skip execution of FieldCoverageMonitor if no item is returned #372

Merged
merged 6 commits into from Mar 3, 2023

Conversation

VladAlex98
Copy link

Closes #366

Added another setting called SPIDERMON_FIELD_COVERAGE_SKIP_IF_NO_ITEM for FieldCoverageMonitor:

  • If this setting is enabled and we don't have any items scraped, it will not raise error anymore, it will send a message instead.

Copy link
Collaborator

@rennerocha rennerocha left a comment

Choose a reason for hiding this comment

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

Hello @VladAlex98 ,
Thanks for your contribution. I added a suggestion so the logs returned for the user would looks nicer and more consistent with other monitor results logs that we have in other built-in monitors.
Also, do not forget to add unit tests for your change and update the documentation of FieldCoverageMonitor adding this new setting.

@@ -482,6 +482,16 @@ def run(self, result):
raise NotConfigured(
"To enable field coverage monitor, set SPIDERMON_ADD_FIELD_COVERAGE=True in your project settings"
)

skip_no_items = self.crawler.settings.get('SPIDERMON_FIELD_COVERAGE_SKIP_IF_NO_ITEM', False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion If we are skipping a test, in my opinion we should use the same pattern as we have in BaseStatMonitor (see code here).

Calling self.skipTest("No items returned") inside FieldCoverageMonitor.test_check_if_field_coverage_rules_are_met if the setting is set and there is no items returned (instead of just return inside FieldCoverageMonitor.run) will make the logs look nicer as:

2022-12-15 15:55:43 [quotes] INFO: [Spidermon] ------------------------------ MONITORS ------------------------------
2022-12-15 15:55:43 [quotes] INFO: [Spidermon] Field Coverage Monitor/test_check_if_field_coverage_rules_are_met... SKIPPED (No items returned)
2022-12-15 15:55:43 [quotes] INFO: [Spidermon] ----------------------------------------------------------------------

An alternative then would be:

def test_check_if_field_coverage_rules_are_met(self):
    skip_if_no_items = self.crawler.settings.get('SPIDERMON_FIELD_COVERAGE_SKIP_IF_NO_ITEM', False)
    items_scraped = self.data.stats.get('item_scraped_count', 0
    if skip_if_no_items and items_scraped == 0:
        self.skipTest("No items were scraped")

    # continue with existing code

@rennerocha rennerocha added this to the 1.18.0 milestone Dec 15, 2022
@VladAlex98
Copy link
Author

Hi @rennerocha, thanks for the code review. I updated the code, added docs and some tests. Please let me know if you need to add something else.

@VladAlex98 VladAlex98 force-pushed the issue-366-skip_fieldcoveragemonitor_no_item_returned branch from 6ee6409 to 977510f Compare February 4, 2023 12:14
@VladAlex98 VladAlex98 force-pushed the issue-366-skip_fieldcoveragemonitor_no_item_returned branch from 977510f to 875de5e Compare February 28, 2023 09:30
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (558aace) 76.41% compared to head (a918c16) 76.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   76.41%   76.44%   +0.02%     
==========================================
  Files          76       76              
  Lines        3193     3197       +4     
  Branches      378      379       +1     
==========================================
+ Hits         2440     2444       +4     
  Misses        683      683              
  Partials       70       70              
Impacted Files Coverage Δ
spidermon/contrib/scrapy/monitors/monitors.py 97.89% <100.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@VMRuiz
Copy link
Collaborator

VMRuiz commented Feb 28, 2023

Hello @rennerocha , is this good now?

Copy link
Member

@curita curita left a comment

Choose a reason for hiding this comment

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

Changes look good!

I mentioned a small suggestion, but otherwise, the functionality makes sense to me.

spidermon/contrib/scrapy/monitors/monitors.py Outdated Show resolved Hide resolved
@VMRuiz VMRuiz dismissed rennerocha’s stale review March 3, 2023 09:05

Requested changes were implemented

@VMRuiz VMRuiz merged commit 234da8d into master Mar 3, 2023
@VMRuiz VMRuiz deleted the issue-366-skip_fieldcoveragemonitor_no_item_returned branch July 10, 2023 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to skip execution of FieldCoverageMonitor if no item is returned
4 participants