search

SimonKagstrom / kcov

Code coverage tool for compiled programs, Python and Bash which uses debugging information to collect and report data without special compilation options
http://simonkagstrom.github.io/kcov/
GNU General Public License v2.0
709 stars 109 forks source link

tests: refactor the test runner #434

Closed perillo closed 4 months ago

perillo commented 4 months ago

Refactor the tests

CHANGES

Additional notes

tests: refactor the test runner

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.

perillo commented 4 months ago

Ops, I forgot to pull #433.

SimonKagstrom commented 4 months ago

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[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

perillo commented 4 months ago

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

perillo commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

For code signing I found

SimonKagstrom commented 4 months ago

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!

perillo commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

I think I'd prefer test/tools

dridi commented 2 weeks ago

As I am working on a kcov update for Fedora, I wanted to thank @perillo for his work on the test suite that I never had time to investigate. I'm done reviewing changes between the v42 and v43 tags and I'm almost positive that this pull request is the one that turns the table on my machine.

It went from almost nothing working to this on my Fedora 40 laptop:

======================================================================
UNEXPECTED SUCCESS: runTest (test_compiled.collect_no_source.runTest)
----------------------------------------------------------------------
Ran 91 tests in 129.627s

FAILED (skipped=8, expected failures=6, unexpected successes=1)

Running the test suite in an RPM build still fails, but much much less than before, when pretty much nothing used to pass:

Ran 89 tests in 101.262s                                                               

FAILED (failures=12, errors=2, skipped=7, expected failures=6, unexpected successes=1)

The remaining failures are probably caused by compilation and linking hardening flags set on Fedora but again, no time to investigate.

That's it, thanks again for your work on the test suite.

SimonKagstrom commented 2 weeks ago

I concur! @perillo did really great work on the testsuite!

And thanks a lot for working on kcov in Fedora @dridi ! I use Fedora myself on my Linux-laptop, although I obviously use my own build of it :-)

perillo commented 2 weeks ago

@dridi and @SimonKagstrom, I'm glad that these changes are useful.

There are still a few changes I would like to implement:

  1. Use list for args, instead of concatenating strings when calling doXX functions
  2. Replace os.system with subprocess. All errors when calling system are ignored
  3. Log arguments, stdout and stderr to a file, when calling doXX functions. This may help find hidden bugs

Unfortunately these change may introduce bugs.