tests: refactor the test runner #434

perillo · 2024-04-04T09:39:52Z

Refactor the tests

CHANGES

tests: refactor the test runner

The current implementation, using the unittest.main function, is not working correctly with how kcov tests are loaded. One annoying problem is that filtering tests is not possible.

Rewrite the test runner, adding a new libkcov package.

The code from "testbase.py" is moved to libkcov.__init__ (the test API), and the code from "run-tests" is moved to libkcov.main (the test runner).

In the test API, remove the configure method and the global variables, since the configuration paths are now processed by argparse as positional arguments. These paths are validated and then used to initialize each test case, that now has an __init__ method.

In order to make running the tests convenient, add a __main__ entry point to the libkcov package.

Rename the test names; "file.py" is renamed to "test_file.py", since this is the preferred naming conventions.

Tests are loaded by a custom test loader, not derived from unittest.TestCase, since the code is really simple.

Move the "cobertura.py" module to the libkcov package, but without the main code. The main code has been moved to "parse_cobertura", so that it can be used as a tool.

The new implementation make is easy do separate the test API, test runner, tests and tools.

In order to reduce the number of changes, in each test file libkcov is imported using as testbase. It will updated in a separate commit.

Update the "ci-run-tests.sh" script to run the tests with python -m linbkcov. It is necessary to set the PYTHONPATH environment variable.

The behavior of the new test runner implementation should be the same as the old one, with the difference that the order of tests is different. The new order is what users expect.
tests: add support for test filtering

The command line support was already added in 74cdbaa (tests: refactor the test runner). Just ensure that a plain string pattern is converted to fnmatch syntax, as done by unittest.main.
tests: add the --shuffle option to the test runner

When enabled, randomize the execution order of tests, allowing support for detecting inter-test dependencies.
tests: rename testbuild to binaries

The new name is more descriptive, like sources.
tests: rename KcovTestCase to TestCase

The Kcov prefix is redundant, since after this change the qualified name will be libkcov.TestCase.

Use "libkcov" as the name when importing libkcov, instead of using testbase (used for avoiding additional renaming in 74cdbaa).
tests: add the TestLoader.add_test_case method

The new method will make TestLoader more readable.

Additional notes

tests: refactor the test runner

stat: 16 files changed, 251 insertions(+), 111 deletions(-)
Not sure if parse_cobertura works correctly, since I have not tested it
@SimonKagstrom can you test it? Is it still used?
I named the package libkcov. A better name is kcov (or ktest?), but
import kcov may cause confusion with self.kcov in the test case.
I plan to rename libkcov.KcovTestCase to libkcov.TestCase
A renaming is already needed, for renaming import libkcov as testbase to import libcov.
I plan to rename testbuild to binaries.
This is not necessary, but IMHO binaries seems a better name. Let me know if you agree.
Originally I planned to move cobertura to a subpackage:
```
libkcov/cobertura/__init__.py
libkcov/cobertura/__main__.py
```
but I changed idea, since I don't know if that code is still used, and parse_cobertura may be more convenient to use.

tests: add the --shuffle option to the test runner

After running a the tests with the --shuffle option, I got two error (in two separate runs):

    ======================================================================
    FAIL: runTest (test_compiled_basic.main_test_verify.runTest)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/manlio/src/contrib/kcov/github/perillo/kcov/tests/tools/test_compiled_basic.py", line 112, in runTest
        self.doTest("--verify")
      File "/home/manlio/src/contrib/kcov/github/perillo/kcov/tests/tools/test_compiled_basic.py", line 97, in doTest
        assert cobertura.hitsPerLine(dom, "main.cc", 9) == 1
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    AssertionError

    ======================================================================
    FAIL: runTest (test_bash_linux_only.bash_sh_shebang.runTest)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/manlio/src/contrib/kcov/github/perillo/kcov/tests/tools/test_bash_linux_only.py", line 17, in runTest
        assert cobertura.hitsPerLine(dom, "sh-shebang.sh", 4) == 1
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    AssertionError

I encountered the test_bash_linux_only.bash_sh_shebang.runTest failure a few time with the old test runner, usually when some other test failed.

The current implementation, using the unittest.main function, is not working correctly with how kcov tests are loaded. One annoying problem is that filtering tests is not possible. Rewrite the test runner, adding a new libkcov package. The code from "testbase.py" is moved to libkcov.__init__ (the test API), and the code from "run-tests" is moved to libkcov.main (the test runner). In the test API, remove the configure method and the global variables, since the configuration paths are now processed by argparse as positional arguments. These paths are validated and then used to initialize each test case, that now has an __init__ method. In order to make running the tests convenient, add a __main__ entry point to the libkcov package. Rename the test names; "file.py" is renamed to "test_file.py", since this is the preferred naming conventions. Tests are loaded by a custom test loader, not derived from unittest.TestCase, since the code is really simple. Move the "cobertura.py" module to the libkcov package, but without the main code. The main code has been moved to "parse_cobertura", so that it can be used as a tool. The new implementation make is easy do separate the test API, test runner, tests and tools. In order to reduce the number of changes, in each test file libkcov is imported using as testbase. It will updated in a separate commit. Update the "ci-run-tests.sh" script to run the tests with `python -m linbkcov`. It is necessary to set the PYTHONPATH environment variable. The behavior of the new test runner implementation should be the same as the old one, with the difference that the order of tests is different. The new order is what users expect.

perillo · 2024-04-04T09:46:54Z

Ops, I forgot to pull #433.

SimonKagstrom · 2024-04-04T10:14:02Z

Woah, very nice, thanks a lot!

The parse_cobertura tool was mainly used to try out the python module, so not really used as such. I'm fine with removing it.

codecov · 2024-04-04T10:34:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.68%. Comparing base (c5a463d) to head (3fe69ad).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #434   +/-   ##
=======================================
  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.

perillo · 2024-04-04T10:48:47Z

There are some problems github: Linux amd64 took a lot of time for installing the firefox snap.

I have a few questions about ci.yml

Why the scripts in .github/workflow don't have the executable bit set (excluding ci-run-tests.sh)?

Currently ci.yml has do to chmod +x
The tests are only run on Linux amd64 and FreeBSD.

On MacOS, the Prepare step do
```
chmod u+x .github/workflows/osx-build.sh .github/workflows/ci-run-tests.sh
```
but the tests are not run. Is this intentional?
It seems that the CI use a release build of kcov. On my system I use a debug build.

Maybe this the reason there was a different behavior in the system_mode test?

IMHO I think that the tests on Linux should be run with normal user privilege.

perillo · 2024-04-04T10:52:54Z

Woah, very nice, thanks a lot!

The parse_cobertura tool was mainly used to try out the python module, so not really used as such. I'm fine with removing it.

Originally I was thinking of adding a new test_cobertura.py test, since I assumed that the __main__ code was a test.

For now I will keep parse_cobertura; it may be removed in a later PR.

SimonKagstrom · 2024-04-04T11:13:29Z

I have a few questions about ci.yml

Why the scripts in .github/workflow don't have the executable bit set (excluding ci-run-tests.sh)?
Currently ci.yml has do to chmod +x

Not sure, but better to make them executable then and cleanup ci.yml. Also looks like it's done in multiple places.

The tests are only run on Linux amd64 and FreeBSD.
On MacOS, the Prepare step do
chmod u+x .github/workflows/osx-build.sh .github/workflows/ci-run-tests.sh
but the tests are not run. Is this intentional?

They used to run, but there is an issue with code signing on MacOS: when running kcov the first time, you'll get a popup with something like "The application kcov wants to control other processes, allow?". You'll need to accept this to run kcov, so the tests on MacOS are now disabled in the ci run (since I can't click the popup...). I haven't investigated very well if it's possible to accept via the CLI somehow.

It seems that the CI use a release build of kcov. On my system I use a debug build.
Maybe this the reason there was a different behavior in the system_mode test?
IMHO I think that the tests on Linux should be run with normal user privilege.

Maybe, but as I said before, the system mode tests are not really relevant since it's barely half-implemented anyway. Better to just disable them I think.

And yes, running as non-root is preferred. I don't remember if there was any particular reason to use sudo, but otherwise it can be turned off.

perillo · 2024-04-04T12:13:05Z

For code signing I found

https://github.com/Apple-Actions/import-codesign-certs
https://stackoverflow.com/questions/31039513/how-can-i-skip-code-signing-for-development-builds-in-xcode
You can try adding XCODE_ATTRIBUTE_CODE_SIGN_IDENTITY "" to cmake.

SimonKagstrom · 2024-04-04T12:24:49Z

Yes, the first one sounds like the way to go.

It needs to be signed though, since it uses non-standard "entitlements", i.e., acting as a debugger. Otherwise, the OS will not allow the ptrace etc calls.

Anyway, that's a separate issue to this!

The command line support was already added in 74cdbaa (tests: refactor the test runner). Just ensure that a plain string pattern is converted to fnmatch syntax, as done by unittest.main.

When enabled, randomize the execution order of tests, allowing support for detecting inter-test dependencies.

The new name is more descriptive, like sources.

The Kcov prefix is redundant, since after this change the qualified name will be libkcov.TestCase. Use "libkcov" as the name when importing libkcov, instead of using testbase (used for avoiding additional renaming in 74cdbaa).

The new method will make TestLoader more readable.

perillo · 2024-04-05T06:34:19Z

These are additional features that I planned to implement, but I think it is better to implement them in the test driver (like https://gist.github.com/perillo/cebd9f82094471b241b961a4f869b0ed), instead.

What do you think about reimplementing it in Python and push to master, in tools/run-test or tests/tools/run-test ?

Here are the additional features:

add the `--count N` argument

This is implemented in go test. It will run the tests N time.
When also adding --failfast, enables testing until failure mode.

The problem is that when also using --shuffle it is not very useful because each run will use the same seed. Better to let the test driver do it.

add the `--work` flag

This is implemented in go test.
When --work is set, it will print the temporary working directory before starting the tests, and will not clean the directory. When not set, it will remove the directory.

One problem is that in order to work, the temporary working directory MUST be created by the test runner. The solution is to remove the outdir positional argument and create a temporary directory.

The second problem is that I would like to set the TMPDIR environment variable to work_dir, but the easy way of using os.putenv does not work, since it may cause memory leaks (see https://docs.python.org/3/library/os.html#os.putenv).

The third problem is that currently ci.yml uses /tmp and it is fine.

IMHO it is better if this is done by the test driver (the test driver is only used when testing locally).

I will implement these features in a separate PR, so this PR can be merged.

SimonKagstrom · 2024-04-05T06:38:15Z

Yes, that sounds like neat features! Adding them in the run-test script sounds fine to me.

Anyway, if you feel this PR is now ready, I'll gladly merge it

perillo · 2024-04-05T07:47:32Z

Yes, that sounds like neat features! Adding them in the run-test script sounds fine to me.

Do you prefer the tools/ directory (in the project root) or the tests/tools directory?

Anyway, if you feel this PR is now ready, I'll gladly merge it

For me it is ready to be merged.

Thanks.

SimonKagstrom · 2024-04-05T12:42:17Z

I think I'd prefer test/tools

perillo force-pushed the refactor-tests branch from fc1ac75 to 74cdbaa Compare April 4, 2024 09:46

perillo added 4 commits April 4, 2024 16:03

tests: add support for test filtering

c405b5a

The command line support was already added in 74cdbaa (tests: refactor the test runner). Just ensure that a plain string pattern is converted to fnmatch syntax, as done by unittest.main.

tests: add the --shuffle option to the test runner

167139e

When enabled, randomize the execution order of tests, allowing support for detecting inter-test dependencies.

tests: rename testbuild to binaries

adea17e

The new name is more descriptive, like sources.

tests: rename KcovTestCase to TestCase

1a97e1c

The Kcov prefix is redundant, since after this change the qualified name will be libkcov.TestCase. Use "libkcov" as the name when importing libkcov, instead of using testbase (used for avoiding additional renaming in 74cdbaa).

SimonKagstrom approved these changes Apr 5, 2024

View reviewed changes

tests: add the TestLoader.add_test_case method

3fe69ad

The new method will make TestLoader more readable.

SimonKagstrom merged commit b374103 into SimonKagstrom:master Apr 5, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: refactor the test runner #434

tests: refactor the test runner #434

perillo commented Apr 4, 2024 •

edited

perillo commented Apr 4, 2024

SimonKagstrom commented Apr 4, 2024

codecov bot commented Apr 4, 2024 •

edited

perillo commented Apr 4, 2024

perillo commented Apr 4, 2024

SimonKagstrom commented Apr 4, 2024

perillo commented Apr 4, 2024

SimonKagstrom commented Apr 4, 2024

perillo commented Apr 5, 2024

SimonKagstrom commented Apr 5, 2024

perillo commented Apr 5, 2024

SimonKagstrom commented Apr 5, 2024

tests: refactor the test runner #434

tests: refactor the test runner #434

Conversation

perillo commented Apr 4, 2024 • edited

Refactor the tests

CHANGES

Additional notes

tests: refactor the test runner

tests: add the --shuffle option to the test runner

perillo commented Apr 4, 2024

SimonKagstrom commented Apr 4, 2024

codecov bot commented Apr 4, 2024 • edited

Codecov Report

perillo commented Apr 4, 2024

perillo commented Apr 4, 2024

SimonKagstrom commented Apr 4, 2024

perillo commented Apr 4, 2024

SimonKagstrom commented Apr 4, 2024

perillo commented Apr 5, 2024

add the --count N argument

add the --work flag

SimonKagstrom commented Apr 5, 2024

perillo commented Apr 5, 2024

SimonKagstrom commented Apr 5, 2024

perillo commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited

add the `--count N` argument

add the `--work` flag