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

Improve tests code more #431

Merged
merged 6 commits into from
Mar 27, 2024

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Mar 26, 2024

Additional improvements to kcov test suite.

  • tests: validate testbase.configure arguments

    Ensure that all the paths in testbase.configure are not empty and
    normalized.

    Ensure that outbase is not the current working directory.

  • tests: convert simple % style string format to f-string

    Convert old formatting style to f-string style, using
    python3 -m libcst.tool codemod convert_percent_format_to_fstring.ConvertPercentFormatStringCommand

  • tests: remove unnecessary call to setUp method.

    It is not necessary because setUp is called automatically by
    TestCase.run.

  • tests: use skipTest instead of print and return

    Using skipTest is the preferred method for skipping tests.
    Now the test message is printed on the same line.

  • tests: override TestCase.tearDown in testbase.py

    Use os.makedirs to create the kcov directory in setUp and shutil.rmtree
    to remove it in tearDown.

    Don't use the shell, since errors are not propagated.

  • Ensure the "kcov-kcov" directory uses a fixed path

    In ci.yml, codecov assumes that the output directory is
    "/tmp/kcov-kcov".

    Don't assume that outbase is always "/tmp", and instead use the
    "/tmp/kcov-kcov" path.

NOTES

  • I used tools to automate refactoring, like LibCST and sed.

  • LibCTS codemod for converting old format to f-string unfortunately added parenthesis in some non simple statement. Only later I found that there was another tool flynt but I have not used it to see if there are differences in formatting.

  • When removing calls to setUp method, I also inadvertently removed calls to setUp method inside the doTest method. This is fine, since doTest is always called from the runTest method so setUp is always called.

TODO

I noted that doShell is used only two times; other code using the shell use system.

Additionally, both doShell and system does not propagate errors, so I plan to make this more robust in a separate PR. I also plan to group shell commands, instead of spawning a subprocess for every command line.

perillo added 6 commits March 26, 2024 10:47
Ensure that all the paths in testbase.configure are not empty and
normalized.

Ensure that outbase is not the current working directory.
Convert old formatting style to f-string style, using
python3 -m libcst.tool codemod convert_percent_format_to_fstring.ConvertPercentFormatStringCommand
It is not necessary because setUp is called automatically by
TestCase.run.
Using skipTest is the preferred method for skipping tests.
Now the test message is printed on the same line.
Use os.makedirs to create the kcov directory in setUp and shutil.rmtree
to remove it in tearDown.

Don't use the shell, since errors are not propagated.
In ci.yml, codecov assumes that the output directory is
"/tmp/kcov-kcov".

Don't assume that `outbase` is always "/tmp", and instead use the
"/tmp/kcov-kcov" path.
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.68%. Comparing base (5ae73ae) to head (505cb60).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #431   +/-   ##
=======================================
  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 SimonKagstrom merged commit c3b0b2a into SimonKagstrom:master Mar 27, 2024
10 checks passed
@SimonKagstrom
Copy link
Owner

Beautiful! Thanks a lot for taking on the testsuite!

@perillo
Copy link
Contributor Author

perillo commented Mar 27, 2024

Thanks to you for the review.

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