eclipse-ankaios / ankaios

Eclipse Ankaios provides workload and container orchestration for automotive High Performance Computing (HPC) software.
https://eclipse-ankaios.github.io/ankaios/
Apache License 2.0
60 stars 22 forks source link

Empty coverage report when filter matches the complete file path #23

Closed windsource closed 11 months ago

windsource commented 1 year ago

Reported by @inf17101:

The llvm-cov command accidentally produces empty coverage reports if the filename regex matches something in the local path: tools/generate_test_coverage_report.sh.

Current Behavior

In the script tools/generate_test_coverage_report.sh the command contains a filename regex to exclude certain files out of the coverage report.

The command looks like this: RUST_LOG=debug cargo llvm-cov --ignore-filename-regex "test|main|test_utils|cli.rs" "$@"

The documentation of llvm-cov (https://github.com/taiki-e/cargo-llvm-cov#exclude-file-from-coverage) says that the regex can be used to exclude some files.

But the "--ignore-filename-regex" option behaves like an "--ignore-filepath-regex", so whenever a substring in the complete absolute path matches the regex the whole repo is excluded from the coverage scan.

If someone pulls the Ankaios Repo for local testing and put it inside a folder "ankaios_test" the user would get an empty coverage report because "test" is inside the regex filter for the path /home/.../ankaios_test/ankaios/....

This is dangerous, because implementation files can be accidentally excluded as well.

The main problem is, it is a valid run. You wouldn't receive any error message or trace (tried with enabling different trace levels and verbose cli options (-vvv)). Just an empty report is generated.

Expected Behavior

The regex expressions shall be made more explicit for exclusions. The documentation and the name of the cli option indicates more that it matches only against file names or at least the relative path starting inside the repo, instead of the whole file path. This would feel more naturally to not have a dependency of the filters on the parent paths or the repository name itself.

Because this means that a successfully generated coverage report depends on how the user who downloads Ankaios repo names the parts or subfolders of the parent paths.

Steps to Reproduce

While I have found out this confusing behavior when implementing CI/CD for Ankaios Repo, it can be easiliy reproduced with the following steps:

  1. Open a shell in VSCode in the development container
  2. Copy the ankaios repo into another path containing a folder name matching something in the regex above: mkdir /home/vscode/ankaios_test
  3. cp -r ankaios /home/vscode/ankaios_test/ # here "ankaios_test" matches the filter regex "test" in the llvm-cov command
  4. cd /home/vscode/ankaios_test/ankaios
  5. tools/generate_test_coverage_report.sh test --html # run the coverage report in the newly copied repo
  6. ls /home/vscode/ankaios_test/ankaios/target/llvm-cov/html
  7. cat /home/vscode/ankaios_test/ankaios/target/llvm-cov/html/index.html

With step 6 you can see that no sub folder "coverage" is created containing all detailed htmls for agent, ank, server and so on. With step 7 you can check the global html report showing an empty report (only 1 row in the table).

Context (Environment)

Reproducible in the devcontainer in VSCode.

Logs

Missing "coverage" subfolder and empty or small index.html:

image

Additional Information

You can make it work again with just renaming the ankios_test folder that it does not match against something inside the filter regex above: mv ankaios_test ankaiosdev

Then running the coverage script again produces the correct report again:

image

Note: The fact that it repeats the absolute path inside the "coverage" subfolder has given me the hint that it matches against the whole path not just the filename or the relative path starting inside the repo. image

krucod3 commented 11 months ago

Should be done now. Just waiting for review.

krucod3 commented 11 months ago

I think we should add this one to v0.2 too as it does not change any code.

maturar commented 11 months ago

The linked pull request has been reviewed and merged into main.

krucod3 commented 11 months ago

All done here.