Skip to content

RFC: Fix direction ALL test assertions org-wide with pytest approach changed in latest version #2394

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

Closed
pombredanne opened this issue Feb 11, 2021 · 0 comments · Fixed by #2407
Labels

Comments

@pombredanne
Copy link
Member

See
pytest-dev/pytest#3333
https://github.com/pytest-dev/pytest/pull/6673/files

In recent pytest versions 5.x+ the ways asserts failures are reported changed (rather dramatically if I may say) where the order is now:

  • assert result == expected
    before and that was our org-wide convention we use the reverse:
  • assert expected == result

This impacts significantly the way we look at test failures traces: the diffs are now unreadable and you need to make the mental switch that plus means minus and minus means plus to make sense of a failure trace.

I always thought that it is more natural to write assertion in this new way e.g. assert result == expected but that was never the convention in unittest, and pytest and related.
To recover sanity and assert again safely that plus == plus and minus=minus I suggest we adopt the way:

  1. define a new org-wide standard to use ths way
  2. progressively update ALL our many tests to use assert result == expected

As an example of the current problem here, this snippet is a failure trace where we expected an mit license_expression to be detected but instead an unknown license_expression was returned:

E                     "homepage_url": null,
E                     "keywords": [],
E         -           "license_expression": "unknown",
E         ?                                  ^^^^^^^
E         +           "license_expression": "mit",
E         ?                                  ^^^
E                     "md5": null,
E                     "name": "laravel",
E                     "namespace": "laravel",

Yet, the way this looks is that an unknown license was not detected and an mit was returned instead.
Before we bump to use pytest 5.x the same failure was looking this way:

E                     "homepage_url": null,
E                     "keywords": [],
E         -           "license_expression": "mit",
E         ?                                  ^^^
E         +           "license_expression": "unknown",
E         ?                                  ^^^^^^^
E                     "md5": null,
E                     "name": "laravel",
E                     "namespace": "laravel",

and this i how we want things to look again hence the push to reorder all our test assertions

pombredanne added a commit that referenced this issue Feb 24, 2021
pombredanne added a commit that referenced this issue Feb 24, 2021
pombredanne added a commit that referenced this issue Feb 24, 2021
Reorder test assertions #2394 

Signed-off-by: Philippe Ombredanne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant