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

Add coverage report generation for tests #436

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

sandrobonazzola
Copy link
Contributor

@sandrobonazzola sandrobonazzola commented Aug 10, 2023

Added targets to Makefile for getting a coverage report from the unit tests.

Example report:
image

Added targets to Makefile for getting a coverage report from the unit
tests.

Signed-off-by: Sandro Bonazzola <sbonazzo@redhat.com>
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is OK, but me personally I don't like using Makefile more and more, we agreed to use meson in the past, so we should stick to it and if really needed provide some shell scripts to ease execution and extend our dependency on Makefiles

README.developer.md Outdated Show resolved Hide resolved
Copy link
Member

@engelmi engelmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Makefile we keep only for more complex or repetitive commands, otherwise its best to directly use meson. Here the basic flow for getting the test coverage is roughly like this:

meson setup builddir    # if already setup, this is a noop
meson configure builddir -Db_coverage=true
meson test -C builddir
ninja coverage-html -C builddir/src/libhirte

I'd prefer doing a meson configure over a meson setup for setting the option since the latter will have no effect if I've run meson setup with a different (or no) value for the option before. We could also set -Db_coverage=true as default - not sure if that has any negative impact, though.
In this project, it makes only sense to collect the coverage from meson test for the libhirte directy - the other parts will probably have to be tested via integration tests (unfortunately).

I'd only have the test-coverage make target which bundles the snippet above. And have the changes in the readme, of course. What do you think? @sandrobonazzola @mwperina

Copy link
Member

@engelmi engelmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be great to export the test coverage as an artifact in the gh action. For this, I think we need to

@engelmi engelmi linked an issue Aug 10, 2023 that may be closed by this pull request
@sandrobonazzola
Copy link
Contributor Author

It would also be great to export the test coverage as an artifact in the gh action.

I was preparing this as follow up to this PR:

index e481a36..f80a67f 100644
--- a/.github/workflows/unit-tests.yml
+++ b/.github/workflows/unit-tests.yml
@@ -17,13 +17,18 @@ jobs:
       - name: Checkout sources
         uses: actions/checkout@v3
 
+      - name: Install coverage tools
+        run: |
+          sudo apt-get install lcov -y
+          pip install html2text
+
       - name: Building hirte
         run: |
-          make build
+          make build-coverage
 
       - name: Running unit tests
         run: |
-          make test-with-valgrind
+          make test-with-valgrind-coverage
 
       - name: Upload unit test logs
         if: always()
@@ -31,3 +36,15 @@ jobs:
         with:
           name: unit-test-logs
           path: ./builddir/meson-logs/testlog-valgrind.txt
+
+      - name: Upload coverage HTML artifact
+        uses: actions/upload-artifact@v3
+        with:
+          name: coverage
+          path: builddir/meson-logs/coveragereport
+          if-no-files-found: error
+
+      - name: Report coverage results
+        run: |
+          cd builddir/meson-logs/coveragereport/
+          html2text --ignore-images --ignore-links -b 0 --bypass-tables index.html >> $GITHUB_STEP_SUMMARY

@sandrobonazzola
Copy link
Contributor Author

I'd only have the test-coverage make target which bundles the snippet above. And have the changes in the readme, of course. What do you think? @sandrobonazzola @mwperina

ok, what about the test with valgrind?

Signed-off-by: Sandro Bonazzola <sbonazzo@redhat.com>
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@engelmi engelmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@engelmi engelmi merged commit dc42fc4 into eclipse-bluechi:main Aug 10, 2023
10 checks passed
@sandrobonazzola sandrobonazzola deleted the coverage branch August 10, 2023 13:58
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.

Collect coverage for tests
3 participants