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

[test][NFC] Change from nose to pytest (analyzer library) #3926

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

Szelethus
Copy link
Collaborator

@Szelethus Szelethus commented May 30, 2023

Mind that this PR only affects the analyzer library, the others still rely on nose!

Motivation, why pytest

Nose is no longer supported, it is not compatible with python3.9 or newer, and will never be:
nose-devs/nose#1099 (comment)

As python3.10 is the oldest python version available from Ubuntu22.04's package manager by default, it is time to migrate.

We never really relied on nose features (as far as I can tell, we never actually import anything from the nose package), so this change isn't particularly painful (don't let the size of the change fool you, its pretty much just moving code around!). Pytest seems to be the industry standard framework lately, and supports most nose constructs out of the box.

The aim of this change that from the make target side, nothing changes -- you still run analyzer tests with make analyzer, and a specific test file with (having 'functional/skip' as the example):

TEST=tests/functional/skip make test_analyzer_feature

This is basically how granular nose could get. If you wanted to run a specific test case in a test file, you couldn't do that, but you can with pytest:

pytest analyzer/tests/functional/skip -v -k test_analyze_header_with_file_option

Mind that there are many environmental variables that need to be set in addition to the above command, so we need to write new make targets to make this user-friendly, but nevertheless, it is possible.

What changed

Broadly speaking, the change can be divided into 4 parts:

  1. Replacing nose with pytest in the makefiles
  2. Replacing nose config files with pytest files
  3. Replace setup_package/teardown_package with setup_class/teardown_class, and setup/teardown to setup_method and teardown_method.
  4. Fix up individual test environments under analyzer/tools.

For the Makefile and config changes, I hope they are self explanatory. Pytest is a rather painless drop-in replacement on the invocation side.

On the conversion, there is a page that discusses how one can convert nose to pytest:
https://docs.pytest.org/en/7.1.x/how-to/nose.html
It is stated (and is true) that pytest supports most, but not quite all features in nose. {setup, teardown}_package is not supported, but it turns out that we dedicate a package to every test class, so simply switching to {setup, teardown}_class was sufficient. That accounts for the vast majority of the code change. That is the only meaningful structural change -- setup->setup_method and the teardown variant was really just a simple rename.

At last, you can notice that some __init__.py files are copied over from analyzer/test/__init__.py. Frankly, I'm not sure how the tests worked previously without these files setting up these variables properly, but this skeleton fell out of the closet now.

-> Motivation, why pytest

Nose is no longer supported, it is not compatible with python3.9, and
will never be:
nose-devs/nose#1099 (comment)

As python3.10 is the oldest python version available from
Ubuntu22.04's package manager by default, it is time to migrate.

We never really relied on many nose features (as far as I can tell, we
never actually import anything from the nose package), this change isn't
particularly painful (don't let the size of the change fool you, its
pretty much just moving code around!). Pytest seems to be the industry
standard framework lately, and supports most nose constructs out of the box.

The aim of this change that from the make target side, nothing changed
-- you still analyzer tests with 'make analyzer', a specific test file
with (having 'functional/skip' as the example):

'TEST=tests/functional/skip make test_analyzer_feature'

This is basically how granular nose could get. If you wanted to run a
specific test case in a test file, you couldn't do that, but you can
with pytest:

pytest analyzer/tests/functional/skip -v -k test_analyze_header_with_file_option

Mind that there are many environmental variables that needs to be set
in addition to the above command, so we need to write new make targets
to make this user-friendly, but nevertheless, it is possible.

-> What changed

Broadly speaking, the change can be divided into 4 parts:

1. Replacing nose with pytest in the makefiles
2. Replacing nose config files with pytest files
3. Replace `setup_package`/`teardown_package` with
   `setup_class`/`teardown_class`, and `setup`/`teardown` to
   `setup_method` and `teardown_method`.
4. Fix up individual test environments under `analyzer/tools`.

For the Makefile and config changes, I hope they are self explanatory.
Pytest is a rather painless drop-in replacement on the invocation side.

On the conversion, there is a page that discusses how one can convert
nose to pytest:
https://docs.pytest.org/en/7.1.x/how-to/nose.html
It is stated (and is true) that pytest supports most, but not quite all
features in nose. `{setup, teardown}_package` is not supported, but it
turns out that we dedicate a package to every test class, so simply
switching to `{setup, teardown}_class` was sufficient. That accounts for
the vast majority of the code change. That is the only meaningful
structural change -- `setup`->`setup_method` and the teardown variant
was really just a simple rename.

At last, you can notice that some `__init__.py` files are copied over
from `analyzer/test/__init__.py`. Frankly, I'm not sure how the tests
worked previously without these files setting up these variables
properly, but this skeleton fell out of the closet now.
@Szelethus Szelethus added test ☑️ Adding or refactoring tests usability 👍 Usability-related features dependencies 📦 Pull requests that update a dependency file labels May 30, 2023
@Szelethus Szelethus added this to the release 6.22.2 milestone May 30, 2023
@Szelethus Szelethus added refactoring 😡 ➡️ 🙂 Refactoring code. and removed usability 👍 Usability-related features labels May 30, 2023
Copy link
Contributor

@bruntib bruntib left a comment

Choose a reason for hiding this comment

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

This is a great improvement, I like this change a lot! The modifications look good to me, so it is close to be merged.

We have some scripts which generate a template test in case someone wants to introduce a new one:
https://github.com/Ericsson/codechecker/tree/master/scripts/test
https://github.com/Ericsson/codechecker/tree/master/web/tests/functional/func_template.
Could you, please, apply these changes in the templates too? Maybe this will only be relevant when the web tests are migrated too.

Thank you!

Comment on lines +8 to +9
# FIXME: Pretty please comment this horrible setting out, I hate it with a
# passion
Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand it can be useful many times that the failed test is right at the end of all verbose logs and you don't need to waste time by locating them in a many megabytes long log message. I agree, though, that pytest is presenting errors much nicer and the localization of errors is much easier.

@Szelethus
Copy link
Collaborator Author

If you would like to check out how the output (without colors) looks like: https://github.com/Ericsson/codechecker/actions/runs/5122929933/jobs/9212716021?pr=3926

@bruntib bruntib merged commit c8db700 into Ericsson:master Jun 6, 2023
Szelethus added a commit to Szelethus/codechecker that referenced this pull request Jun 6, 2023
Szelethus added a commit to Szelethus/codechecker that referenced this pull request Jun 6, 2023
Szelethus added a commit to Szelethus/codechecker that referenced this pull request Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Pull requests that update a dependency file refactoring 😡 ➡️ 🙂 Refactoring code. test ☑️ Adding or refactoring tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants