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

Additional test fixes #435

Merged

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Apr 5, 2024

  • tests: fix incorrect code in test_compiled.py

    The SignalBase method of the SignalBase class is never called; probably it should have be the __init__ method. Remove it, since self.m_self is always set on subclasses.

    In collect_and_report_only, use Exception, instead of BaseException, since the former does not catch GeneratorExit, KeyboardInterrupt and SystemExit.

    In system_mode_can_record_and_report_binary, the test fails as expected, but the cause of the error is:
    NameError: name 'time' is not defined

    Import the time module.

    Errors reported by ruff check.

  • tests: make naming convention consistent in test_compiled.py

    Rename Pie to pie.

    I forgot to apply this change in d2403a7 (tests: use consistent naming convention in test cases).

  • tests: fix typo in test_python.py

    Rename python_coverage.runTestTest to python_coverage.runTest.

  • tests: fix test_compiled.address_sanitizer_coverage test

    Restore building of the sanitizer-coverage executable in cmake, and just skip the test if it was not built with clang.

    The original code made it hard to understand the purpose of the test.

  • tests: remove obsolete code from tests/CMakeLists

    cmake 3.12 supports the POSITION_INDEPENDENT_CODE property, so there is no need for the version check.

NOTES

I'm not fully sure about commit "tests: fix test_compiled.address_sanitizer_coverage test".

perillo added 5 commits April 5, 2024 15:35
The `SignalBase` method of the SignalBase class is never called;
probably it should have be the __init__ method.  Remove it, since
`self.m_self` is always set on subclasses.

In `collect_and_report_only`, use `Exception`, instead of
`BaseException`, since the former does not catch `GeneratorExit`,
`KeyboardInterrupt` and `SystemExit`.

In `system_mode_can_record_and_report_binary`, the test fails as
expected, but the cause of the error is:
    NameError: name 'time' is not defined

Import the `time` module.

Errors reported by `ruff check`.
Rename `Pie` to `pie`.

I forgot to apply this change in d2403a7 (tests: use consistent naming
convention in test cases).
Rename `python_coverage.runTestTest` to `python_coverage.runTest`.
Restore building of the sanitizer-coverage executable in CMake, and just
skip the test if it was not built with clang.

The original code made it hard to understand the purpose of the test.
cmake 3.12 supports the POSITION_INDEPENDENT_CODE property, so there is
no need for the version check.
@perillo perillo changed the title Additional test improvements Additional test fixes Apr 5, 2024
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.68%. Comparing base (b374103) to head (449fec0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
- Coverage   65.70%   65.68%   -0.03%     
==========================================
  Files          58       58              
  Lines        4514     4514              
  Branches     4171     4171              
==========================================
- Hits         2966     2965       -1     
- Misses       1548     1549       +1     

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

@SimonKagstrom
Copy link
Owner

Thanks, looks good!

The sanitizer coverage collection is also not completely working, and is probably not very interesting anyway (since it still requires special compile options, which is sort of the point of kcov to start with).

And kudos for the very good PR descriptions by the way!

@SimonKagstrom SimonKagstrom merged commit b270dce into SimonKagstrom:master Apr 6, 2024
9 of 10 checks passed
@perillo
Copy link
Contributor Author

perillo commented Apr 6, 2024

Thanks, looks good!

The sanitizer coverage collection is also not completely working, and is probably not very interesting anyway (since it still requires special compile options, which is sort of the point of kcov to start with).

Yes, I was aware that the sanitizer is not working correctly. The problem is that the old test used assert False to force the test to fail. Now the code is a bit more easy to understand.

And kudos for the very good PR descriptions by the way!

Thanks!
Also, thank you for taking your time to review all the changes that I proposed.

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