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.

Sign up for GitHub

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

Jump to bottom

Fix issue 99 #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issue 99 #137

wants to merge 1 commit into from

Conversation

FilippoPetroli
Copy link
Member

Solves issue #99

@codecov
Copy link

codecov bot commented Mar 9, 2023 •
edited
Loading

Codecov Report

Merging #137 (357741d) into master (b313ec1) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 357741d differs from pull request most recent head 598a73c. Consider uploading reports for the commit 598a73c to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   88.83%   88.81%   -0.02%     
==========================================
  Files          14       14              
  Lines         770      769       -1     
==========================================
- Hits          684      683       -1     
  Misses         86       86
Impacted Files Coverage Δ
scrapy_poet/api.py 100.00% <100.00%> (ø)

@FilippoPetroli
Copy link
Member Author

Hi @kmike
I fixed the issue. I saw that there are some silenced test that mention DummyResponse so maybe there are edges to trim. I wrote a basic test but if you prefer, I can write more tests.
Thanks

cc @felipeboffnunes

kmike
kmike reviewed Mar 9, 2023
View reviewed changes
tests/test_downloader.py
assert dummy.status == 300

dummy = DummyResponse("https://google.com", 400)
assert dummy.status == 400
Copy link
Member

@kmike kmike Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey! The original issue was that .replace doesn't work; could you please add a test for this, or maybe even change this test to check .replace?

Users shouldn't be creating DummyResponse instances themselves, but they might have code which calls response.replace.

@kmike
Copy link
Member

kmike commented Mar 9, 2023

Looks good overall, thanks @FilippoPetroli!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmike kmike kmike left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FilippoPetroli @kmike