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

tests: improve the test suite implementation #429

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Mar 17, 2024

This is the first of a series of PR to improve the test suite readability and reliability.

  • tests: ensure python 3.8

    This version supports features that will help improving the current
    implementation:

    • subprocess.run
    • fstring
    • positional-only parameters

    Update tests.CMakeLists.txt to find the Python3 package.

    Add the ruff.toml file. Line lenght has been set to 100, and isort is
    enabled for lint.

    See also
    https://devguide.python.org/versions/

  • tests: run "ruff format tests/tools"

    Run "ruff format tests/tools" and "ruff format tests/tools/run-tests"

    This commit changes a lot of code

    statistics:
    tests/tools/accumulate.py | 166 +++++++++--
    tests/tools/bash.py | 327 +++++++++++++++++++---
    tests/tools/bash_linux_only.py | 42 ++-
    tests/tools/basic.py | 19 +-
    tests/tools/compiled.py | 463 +++++++++++++++++++++++++------
    tests/tools/compiled_basic.py | 81 +++++-
    tests/tools/filter.py | 51 +++-
    tests/tools/parse_cobertura.py | 19 +-
    tests/tools/python.py | 127 ++++++++-
    tests/tools/run-tests | 6 +-
    tests/tools/system_mode.py | 32 ++-
    tests/tools/testbase.py | 23 +-
    12 files changed, 1153 insertions(+), 203 deletions(-)

  • tests: run "ruff check --fix tests/tools"

    Run "ruff check --fix tests/tools" and
    "ruff check --fix tests/tools/run-tests"

    Apply safe fixes.

  • tests: run "ruff check --fix --unsafe-fixes tests/tools"

    Run "ruff check --fix --unsafe-fixes tests/tools" and
    "ruff check --fix --unsafe-fixes tests/tools/run-tests"

    Apply unsafe fixes. These are actually safe.

    These are additional errors reported by ruff:

    tests/tools/compiled.py:217:9: F821 Undefined name self
    tests/tools/compiled.py:321:9: E722 Do not use bare except
    tests/tools/compiled.py:685:9: F821 Undefined name time
    tests/tools/compiled.py:689:9: F821 Undefined name time
    tests/tools/system_mode.py:61:9: E722 Do not use bare except

Most of the code change has be done automatically. Also note that some change are not optimal, but I avoided manual changes.

Since review will be very hard and you should not trust me, I suggest to @SimonKagstrom that

Only merge the first commit.
Add a commit for each of the following commands:

> ruff format tests/tools
> ruff format tests/tools/run-tests
> ruff check --fix tests/tools
> ruff check --fix tests/tools/run-tests
> ruff format tests/tools
> ruff format tests/tools/run-tests

I used ruff 0.3.3 but it should be ok to use a different version.

Formatting the code will make future changes more easy.
As an example, I plan to change how the command line is created for subprocess.
Currently is is a string, where arguments are concatenated together; it is terrible!

perillo added 4 commits March 17, 2024 17:52
This version supports features that will help improving the current
implementation:
  - subprocess.run
  - f-strings
  - positional-only parameters

Update tests.CMakeLists.txt to find the Python3 package.

Add the ruff.toml file.  Line lenght has been set to 100, and isort is
enabled for lint.
Run "ruff format tests/tools" and "ruff format tests/tools/run-tests"

This commit changes a lot of code.
Run "ruff check --fix tests/tools" and
"ruff check --fix tests/tools/run-tests"

Apply safe fixes.
Run "ruff check --fix --unsafe-fixes tests/tools" and
"ruff check --fix --unsafe-fixes tests/tools/run-tests"

Apply unsafe fixes. These are actually safe.

These are additional errors reported by ruff:

  tests/tools/compiled.py:217:9: F821 Undefined name `self`
  tests/tools/compiled.py:321:9: E722 Do not use bare `except`
  tests/tools/compiled.py:685:9: F821 Undefined name `time`
  tests/tools/compiled.py:689:9: F821 Undefined name `time`
  tests/tools/system_mode.py:61:9: E722 Do not use bare `except`
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.68%. Comparing base (49635e7) to head (2d48999).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #429   +/-   ##
=======================================
  Coverage   65.68%   65.68%           
=======================================
  Files          58       58           
  Lines        4514     4514           
  Branches     4171     4171           
=======================================
  Hits         2965     2965           
  Misses       1549     1549           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SimonKagstrom
Copy link
Owner

I'm OK with merging all of it at once, it's still separated into commits, or did I misunderstand you?

Thanks for doing this!

@perillo
Copy link
Contributor Author

perillo commented Mar 18, 2024

I'm OK with merging all of it at once, it's still separated into commits, or did I misunderstand you?

Thanks for doing this!

What I mean is that this is a big change. Personally I would feel insecure about merging a pull request like this. Thanks for the trust!

@SimonKagstrom
Copy link
Owner

Well, I looked at the commits, and since it only affects the testsuite (which also runs fine), I'm not too worried!

@SimonKagstrom SimonKagstrom merged commit 5ae73ae into SimonKagstrom:master Mar 18, 2024
10 checks passed
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.

2 participants