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

[prerelease] deselected addition to assert_outcomes() is backwards-incompatible #9471

Closed
The-Compiler opened this issue Jan 3, 2022 · 6 comments · Fixed by #9475
Closed
Labels
plugin: pytester related to the pytester builtin plugin type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog
Milestone

Comments

@The-Compiler
Copy link
Member

#9133 added a new deselected parameter to assert_outcomes(), cc @okken.

However, this actually is an incompatible change: Doing e.g. result = testdir.runpytest("-k", "test_not_found_by_ini") followed by result.assert_outcomes(passed=2) worked fine before, but now fails because the now included 'deselected': ... does not equal 'deselected': 0.

This breaks pytest-bdd: pytest-dev/pytest-bdd#466 - I could swear I also saw another project in #9415 fail after fixing the initial issue it had, but then Christmas and stuff came along and now I don't remember which one it was, and of course can't find it anymore.

A (quite) rough search reveals that more projects might be affected by this (excludes to avoid matches in copies of pytest's source code).

Some examples I could dig up (but haven't verified):

I think the change in itself makes sense, but at the same time fixes like pytest-dev/pytest-bdd#470 are a bit cumbersome.

Two questions:

  • What should we do about this for 7.0? (even if the answer just is "live with it and document it as backwards-incompatible in the changelog)
  • What (if anything) should we do about this so that it doesn't happen again for future releases? I guess not much we can do, as long as we want to assume 0 for outcomes which have not been given...
@The-Compiler The-Compiler added type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog plugin: pytester related to the pytester builtin plugin labels Jan 3, 2022
@The-Compiler The-Compiler added this to the 7.0 milestone Jan 3, 2022
@RonnyPfannschmidt
Copy link
Member

@okken would you like to make the deselected a optional parameter to ensure this is caught

i believe we have to make certain new features "explicit optionals" instead of implied optionals

this one is a easy miss, i'm pretty sure at least 3 of us missed this when glancing at the pr before, and we would miss a issue like that again, as making it visible would require a explicit and elaborate design of the matcher to begin with

this type of regression is pretty much only caught by overly detailed test suites, so given the circumstances we rather ought to focus on making it inexpensive to react on them

lets try to make it a actual optional parameter before 7.0 but not get hung up on it in case it turns out tricky

@okken
Copy link
Contributor

okken commented Jan 5, 2022

Optional seems fine.

@okken
Copy link
Contributor

okken commented Jan 5, 2022

Apologies for. Not noticing this question until now. I’m impressed with the pragmatism of the solution.

@okken
Copy link
Contributor

okken commented Jan 5, 2022

Is there more work to do on this, or does #9475 solve this?

@RonnyPfannschmidt
Copy link
Member

@okken the pr (that i jsut reopened) just sillves the immediate regression

there still is need for a better long term plan for that api, as its not exactly lending itself to extension

@okken
Copy link
Contributor

okken commented Jan 14, 2022

This is still open. Is someone waiting for me to do something? If so, what?
Or does someone else need to do something?
Or is this just a lower priority to close than the other issues?
:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: pytester related to the pytester builtin plugin type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants