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.

Sign up for GitHub

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

Jump to bottom

Coverage - 2nd attempt #187

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Coverage - 2nd attempt #187

merged 4 commits into from
Oct 1, 2024

Conversation

mkalcok
Copy link
Contributor

@mkalcok mkalcok commented Oct 1, 2024

after #184 had to be reverted, this is a second attempt at enabling coverage data collection during system tests.

This time the snapcraft.yaml file is static, so build system like launchpad shouldn't have problem using this repository to publish snaps. Instead, the build is controlled via variables in microovn/.build_control file that gets sourced during the snap build.

This PR is a draft. It does not contain doc changes yet as I'd like some feedback on its viability first.

fnordahl
fnordahl previously approved these changes Oct 1, 2024
View reviewed changes
@fnordahl
Copy link
Member

fnordahl commented Oct 1, 2024 •
edited
Loading

after #184 had to be reverted, this is a second attempt at enabling coverage data collection during system tests.

This time the snapcraft.yaml file is static, so build system like launchpad shouldn't have problem using this repository to publish snaps. Instead, the build is controlled via variables in microovn/.build_control file that gets sourced during the snap build.

This PR is a draft. It does not contain doc changes yet as I'd like some feedback on its viability first.

Yes, I think it's a viable path, a couple of comments:

  1. The file augments the snap build environment, should we perhaps keep the file together with snapcraft.yaml? I see no need to tuck it away as a hidden file btw.

  2. Should we augment the version string to make it clear this is a instrumented snap/binary, in case someone manages to toggle this unintentionally.

@mkalcok
Copy link
Contributor Author

mkalcok commented Oct 1, 2024

The file augments the snap build environment, should we perhaps keep the file together with snapcraft.yaml?

The main benefit of having the file in microovn/ directory is that if its content changes, snapcraft will pick it up on it and rebuild the microovn part. If it was in the snap/ and you changed it, you'd just get Skipping build for microovn (already ran)

Should we augment the version string to make it clear this is a instrumented snap/binary

So for the snap version, we are already cutting it pretty close to the character limit of 32 (e.g. 24.03.2+snap9c47bdc598-dirty is 28 chars), but we could include it in the version string that gets outputed by microovn --version

@fnordahl
Copy link
Member

fnordahl commented Oct 1, 2024

The file augments the snap build environment, should we perhaps keep the file together with snapcraft.yaml?

The main benefit of having the file in microovn/ directory is that if its content changes, snapcraft will pick it up on it and rebuild the microovn part. If it was in the snap/ and you changed it, you'd just get Skipping build for microovn (already ran)

That is a fair point, what about microovn/build-aux/environment or something like that then?

Should we augment the version string to make it clear this is a instrumented snap/binary

So for the snap version, we are already cutting it pretty close to the character limit of 32 (e.g. 24.03.2+snap9c47bdc598-dirty is 28 chars), but we could include it in the version string that gets outputed by microovn --version

The snap version is indeed long as-is, if we can watermark the binary that works for me.

mkalcok added 4 commits October 1, 2024 11:43
@mkalcok
snap: Conditional build w/ coverage
3dded5c 
Dynamically influence build of MicroOVN snap via
variables set in 'microovn/.build_control' file.

This allows us to build MicroOVN with coverage instrumentation
on demand, without affecting default builds.

Signed-off-by: Martin Kalcok <[email protected]>
@mkalcok
tests: collecting coverage data
c20e380 
When MicroOVN is built with coverage enabled, system
tests can collect coverage data from MicroOVN client and
daemon binaries executed during tests. By default, the data
is stored locally in ".coverage/<test_name>/<container_name>".

Signed-off-by: Martin Kalcok <[email protected]>
@mkalcok
ci: Collect coverage data during tests
ed3e4b9 
MicroOVN snap built in the CI pipeline now supports
coverage instrumentation in its client and daemon binaries.
Test jobs will use this build to collect coverage
data that gets uploaded as an artifact from each job.

After all tests are executed, a new job will collect and
merge coverage data from all tests and produce coverage
report in Cobertura XML format.

Signed-off-by: Martin Kalcok <[email protected]>
@mkalcok
docs: Code coverage
9d40e7f 
Included topics about building MicroOVN with code coverage
support and how to collect coverage data from system tests.

Signed-off-by: Martin Kalcok <[email protected]>
@mkalcok mkalcok marked this pull request as ready for review October 1, 2024 10:03
@mkalcok mkalcok requested a review from a team as a code owner October 1, 2024 10:03
@mkalcok
Copy link
Contributor Author

mkalcok commented Oct 1, 2024

This approach seems to be working fine. CI results produced cobertura.xml that looks good.

Update since last push:

  • control file moved to microovn/build-aux/environment
  • microovn --version shows -cover suffix if binaries were built with coverage instrumentation

fnordahl
fnordahl approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

@fnordahl fnordahl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fnordahl fnordahl merged commit 69d205b into canonical:main Oct 1, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnordahl fnordahl fnordahl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mkalcok @fnordahl