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

Incompatible with pytest 5.4 and 5.4.1 #4588

Merged
merged 2 commits into from May 25, 2020
Merged

Conversation

altendky
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4588      +/-   ##
==========================================
+ Coverage   84.54%   84.57%   +0.03%     
==========================================
  Files         164      164              
  Lines        9931     9931              
  Branches     1477     1477              
==========================================
+ Hits         8396     8399       +3     
+ Misses       1267     1266       -1     
+ Partials      268      266       -2     
Impacted Files Coverage Δ
scrapy/core/downloader/__init__.py 90.97% <0.00%> (+1.50%) ⬆️
scrapy/utils/trackref.py 85.71% <0.00%> (+2.85%) ⬆️

@altendky
Copy link
Contributor Author

This is following up on pytest-dev/pytest-twisted#93.

@elacuesta
Copy link
Member

elacuesta commented May 20, 2020

Thanks for the work on pytest-dev/pytest-twisted#93 🚀
Should we consider pinning pytest>=5.4.2 instead?

@altendky
Copy link
Contributor Author

I did this because before it was just pytest so I was only banning the actually failing versions instead of introducing a minimum. If this were a new feature rather than a temporary breakage in a couple versions then >= would make sense to me.

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

+1 since there’s no reason to change the minimum.

However, I would suggest including an in-line comment for reference, pointing to our issue or the upstream issue.

@altendky
Copy link
Contributor Author

So my only concern with going ahead with this is that there still might be something funny going on with the added pytest-twisted test when running with qt5reactor. Might be nothing, might be irrelevant, I haven't spent any time on it yet. Anyways, just wanted to mention that. But hey, if more versions are found to be problematic they can be listed later.

@kmike
Copy link
Member

kmike commented May 25, 2020

Thanks so much @altendky for making pytest-twisted working for us!

@kmike kmike merged commit b82a480 into scrapy:master May 25, 2020
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.

None yet

4 participants