Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
configure pytest to be stricter #1040
Conversation
|
Hey
Thanks again, and I'd be open to this PR just fixing the first issue in this list, and maybe a separate PR for the second point if that's something you can give a stronger reason for. |
graingert
force-pushed the
graingert:fix-collection-warning-stricter-pytest
branch
2 times, most recently
from
1ce19ad
to
396c018
It's very possible to accidentally skip tests by adding an secondly it's good to find and explicitly list the warnings that you're ignoring, rather than just having them fruitlessly print out at the end of the suite. thirdly you had a SyntaxWarning that until run with a stricter pytest was not noticed. |
You were previously filtering all of them, in this PR I'm fixing it so you can keep tabs on them |
Fair enough, although "it's good to" is not a tangible benefit. I do see a benefit to having an explicit list as it means we know what warnings we expect and when a new one shows up we're more aware of it. On the other hand, somebody then needs to maintain the list of known warnings and it can get stale. I could go either way
Where was that? This PR looks like a good change, thanks. IMO, parts of Starlette could do with some TLC, and using our test/lint tools to highlight those areas and keep things neat and tidy is a good idea. Please try to keep it to a minimal set of changes, though. If the PR does exactly what it says in the title then I'm happy. |
The missing |
graingert
changed the title
Yeah it's not so bad as you get notified when you need to add new ones, I've got a ticket for xwarns decorator in pytest too |
I understand that this is more work for you, but squeezing extra stuff into PRs makes more work for the reviewer. We also squash commits when we merge, so PRs that change multiple unrelated things make it harder to track down the reasons for changes in git history. |
|
@JayH5 originally these are all related to the increase in strictness of pytest |
enhance pytest strictness to catch warnings that would have prevented thest TestUploadFile file warning restore cov-report and require 100% coverage
graingert
force-pushed the
graingert:fix-collection-warning-stricter-pytest
branch
from
a3c7f2e
to
c923a96
graingert
changed the title
graingert
changed the title
graingert
force-pushed the
graingert:fix-collection-warning-stricter-pytest
branch
from
e6fa6e6
to
c923a96
I'd recommend using a custom |
That might be nice to have, but unless someone has interest in building/maintaining such a thing it is unlikely to happen for now. |
Co-authored-by: Jamie Hewland <jhewland@gmail.com>
|
Looks good. I've suggested a couple of comments so we can hopefully better keep track of the deprecation warnings. It looks like the context/variable ones we can maybe fix in Starlette, but for a different issue/PR. I'm not super clear on where comments can go in setup.cfg files so relying on my syntax highlighter to say those are ok. |
Co-authored-by: Jamie Hewland <jhewland@gmail.com>
Co-authored-by: Jamie Hewland <jhewland@gmail.com>
Co-authored-by: Jamie Hewland <jhewland@gmail.com>
|
We got there eventually |
configure pytest to be stricter and fix all the problems that the increased strictness detects,
eg
tests/test_datastructures::TestUploadFile PytestCollectionWarning