Enable coverage in integration tests #4682

TheRealFalcon · 2023-12-11T21:16:11Z

Proposed Commit Message

rebase

Additional Context

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

TheRealFalcon · 2023-12-11T21:20:01Z

tests/integration_tests/conftest.py

-        ]
-    ):
-        return
+def _collect_logs(instance: IntegrationInstance, log_dir: Path):


To help read the following sections:
Instead of _collect_logs, we instead call a new _collect_artifacts after every test. This function will check what needs to be setup based on the integration settings, then call _collect_logs and/or _collect_coverage accordingly.

aciba90

Thanks, @TheRealFalcon, great work.

As it is right now:

We instrument the cloud-init services with coverage in the tested instance.
Run pytest from test-runner instance.
Pull the coverage metadata files from the tested instance.
Generate the coverage reports in the test-runner instance.

The last step fails if cloud-init is not installed in the test-runner instance because the coverage metadata points to /usr/lib/python3/dist-packages/cloudinit. See: log.txt.

I wonder if it would be better to generate the reports in the tested instance and pull them, as coverage is already installed there.

This would remove the need to install cloud-init in developer's machines and would avoid misalignment between cloud-init versions in both machines, which could provoke this same error (I am not sure if the absence or modification of only one file would lead to this error, but it could be).

holmanb

Nice, thanks! One inline question / and some random comments.

In response to @aciba90's comments:

The last step fails if cloud-init is not installed in the test-runner instance

Is this actually a problem? The package python3-coverage gets installed in tests/integration_tests/instances.py, so when would this happen?

I wonder if it would be better to generate the reports in the tested instance and pull them, as coverage is already installed there.

This would remove the need to install cloud-init in developer's machines and would avoid misalignment between cloud-init versions in both machines, which could provoke this same error (I am not sure if the absence or modification of only one file would lead to this error, but it could be).

I think you meant s/cloud-init/python3-coverage/g here. I agree this might forcibly require the host and guest to have the same coverage version for this to work. Generation on the host would be better I think and therefore also wouldn't require local install of coverage.

holmanb · 2024-01-03T16:21:48Z

tests/integration_tests/assets/enable_coverage.py

+        exit(1)
+
+# Prepend the ExecStart= line with 'python3 -m coverage run'
+for service in services:


I've done similar things (more ad-hoc) for inserting profiler commands into the service files. At some point I'll probably steal this code or make it more generic to do that in automatically in tox (unless someone else wants to do it)

holmanb · 2024-01-03T16:29:15Z

tests/integration_tests/util.py


 import pytest

 from cloudinit.subp import subp
-from tests.integration_tests.instances import IntegrationInstance
+
+if TYPE_CHECKING:


Why this? Performance optimization? Can we add a comment or something to the commit message? Also if this isn't required for enabling coverage maybe it should go in its own commit?

Avoid circular import. I almost made it a separate commit...so...I guess I will 😄

Could we please include a comment that it's for avoiding circular import?

TheRealFalcon · 2024-01-15T22:43:18Z

The last step fails if cloud-init is not installed in the test-runner instance because the coverage metadata points to /usr/lib/python3/dist-packages/cloudinit. See: log.txt.

Thanks! I had the cloud-init package installed locally, so I hadn't noticed this

This would remove the need to install cloud-init in developer's machines and would avoid misalignment between cloud-init versions in both machines, which could provoke this same error

You've identified two different issues:

The need to install the cloud-init package to ensure matching paths
Misalignment between cloud-init versions

For 1, coverage has a configuration option to align a directory on your local machine with paths from coverage files. I pushed a change (see tox.ini) that does this.

For 2, that is a valid concern, but it's not one that I'm super concerned about. If there's a mismatch in source, our coverage numbers might be slightly off, but in most cases not enough to make a considerable difference. If we want to avoid this, we can ensure that the code we're testing is the same as the code we're launching from.

I wonder if it would be better to generate the reports in the tested instance and pull them, as coverage is already installed there.

I'm not sure there's a way as easy way to do this. coverage has the option to combine binary results, but I haven't seen an option for combining generated reports. The other option would be to launch one more instance after all tests have run, push all the coverage results there, then have that instance generate them, but that seems like a lot of overhead for something like this. If it's that vs misalignment, I prefer the misalignment.

aciba90 · 2024-01-16T09:36:01Z

Thanks for the explanation and addressing 1. I agree, 2 is not worth handling.

I get now the following error:

2024-01-16 10:27:25 INFO      integration_testing:conftest.py:363 Generating coverage report                                                                                                                       
/home/aciba/canonical/wt/cloud-init/reviews/.tox/integration-tests/lib/python3.11/site-packages/coverage/data.py:171: CoverageWarning: Couldn't use data file '/tmp/cloud_init_test_logs/240116102518/.coverage.tes
ts-integration_tests-test_paths-TestHonorCloudDir-test_honor_cloud_dir': Looks like a coverage 4.x data file. Are you mixing versions of coverage?

Misalignment between inner and outer coverage versions?

Full test session: https://pastebin.canonical.com/p/gShy25PkSq/

TheRealFalcon · 2024-01-16T17:10:13Z

I pushed two new commits to install via pip rather than using python3-coverage.

If the PR is approved, I plan on moving the refactor commit earlier and squashing the fixup commit, but I left them in the current order for easier review.

aciba90

LGTM, thanks.

) Integration tests allow for installing a new version of cloud-init with the option of snapshotting the image after install. After taking a snapshot, it will then set the snapshot as the default for *all* future launches in this test run. This is also used by the conftest to setup the initial image, but it should not be used anywhere else. Rather than having it as an option to the install_new_cloud_init method, remove the option and make the confest set the global snapshot instead. This removes a footgun when writing other tests that need to install a different version of cloud-init.

`get_top_level_dir` and `cloud_init_project_dir` have uses in integration tests. Move them up to a top-level test helper file accordingly.

Integration tests allow for installing a new version of cloud-init with the option of snapshotting the image after install. After taking a snapshot, it will then set the snapshot as the default for *all* future launches in this test run. This is also used by the conftest to setup the initial image, but it should not be used anywhere else. Rather than having it as an option to the install_new_cloud_init method, remove the option and make the confest set the global snapshot instead. This removes a footgun when writing other tests that need to install a different version of cloud-init.

`get_top_level_dir` and `cloud_init_project_dir` have uses in integration tests. Move them up to a top-level test helper file accordingly.

TheRealFalcon commented Dec 11, 2023

View reviewed changes

TheRealFalcon force-pushed the int-test-cov branch from b64c70b to 9001f07 Compare December 11, 2023 21:21

aciba90 self-assigned this Dec 13, 2023

aciba90 reviewed Dec 15, 2023

View reviewed changes

holmanb reviewed Jan 3, 2024

View reviewed changes

TheRealFalcon force-pushed the int-test-cov branch from 9001f07 to d33dee0 Compare January 15, 2024 22:26

TheRealFalcon force-pushed the int-test-cov branch from d6daa1c to a3e1d40 Compare January 16, 2024 20:11

aciba90 approved these changes Jan 17, 2024

View reviewed changes

TheRealFalcon added 4 commits January 17, 2024 12:30

docs: fix cloud-init single param docs (canonical#4682)

3e00e4d

test: Move unit test helpers to global test helpers (canonical#4682)

9265772

`get_top_level_dir` and `cloud_init_project_dir` have uses in integration tests. Move them up to a top-level test helper file accordingly.

test: Enable coverage in integration tests (canonical#4682)

673bb73

TheRealFalcon force-pushed the int-test-cov branch from a3e1d40 to 673bb73 Compare January 17, 2024 18:31

TheRealFalcon merged commit 4940351 into canonical:main Jan 17, 2024
26 of 28 checks passed

TheRealFalcon added a commit that referenced this pull request Jan 17, 2024

docs: fix cloud-init single param docs (#4682)

5b5eb8b

TheRealFalcon added a commit that referenced this pull request Jan 17, 2024

test: Move unit test helpers to global test helpers (#4682)

f1ca01c

`get_top_level_dir` and `cloud_init_project_dir` have uses in integration tests. Move them up to a top-level test helper file accordingly.

TheRealFalcon deleted the int-test-cov branch January 17, 2024 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable coverage in integration tests #4682

Enable coverage in integration tests #4682

TheRealFalcon commented Dec 11, 2023

TheRealFalcon Dec 11, 2023

aciba90 left a comment

holmanb left a comment •

edited

Loading

holmanb Jan 3, 2024

holmanb Jan 8, 2024

holmanb Jan 3, 2024

TheRealFalcon Jan 3, 2024

holmanb Jan 8, 2024

TheRealFalcon commented Jan 15, 2024

aciba90 commented Jan 16, 2024

TheRealFalcon commented Jan 16, 2024

aciba90 left a comment

Enable coverage in integration tests #4682

Enable coverage in integration tests #4682

Conversation

TheRealFalcon commented Dec 11, 2023

Proposed Commit Message

Additional Context

Merge type

TheRealFalcon Dec 11, 2023

Choose a reason for hiding this comment

aciba90 left a comment

Choose a reason for hiding this comment

holmanb left a comment • edited Loading

Choose a reason for hiding this comment

holmanb Jan 3, 2024

Choose a reason for hiding this comment

holmanb Jan 8, 2024

Choose a reason for hiding this comment

holmanb Jan 3, 2024

Choose a reason for hiding this comment

TheRealFalcon Jan 3, 2024

Choose a reason for hiding this comment

holmanb Jan 8, 2024

Choose a reason for hiding this comment

TheRealFalcon commented Jan 15, 2024

aciba90 commented Jan 16, 2024

TheRealFalcon commented Jan 16, 2024

aciba90 left a comment

Choose a reason for hiding this comment

holmanb left a comment •

edited

Loading