tests: add a unit test suite and measure coverage - #22
Merged
Merged
Conversation
insatomcat
marked this pull request as draft
August 2, 2026 18:01
insatomcat
marked this pull request as ready for review
August 3, 2026 07:56
setup_ovs.py imports yaml to read .yaml and .yml configuration files, but the project declared no dependency at all. An installed package therefore raised ModuleNotFoundError on those files unless PyYAML happened to be present for another reason. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
The project had no test at all, so the two OpenSSF gold coverage criteria (test_statement_coverage90 and test_branch_coverage80) could not be evaluated: they ask for a measured figure, not for a suite that merely exists. Add a pytest suite covering the five modules. Everything that touches the system is mocked (subprocess, sysfs, /proc, the network stack), so the suite needs neither root nor a cluster and runs anywhere. It measures 99.09 percent of statements and 98.65 percent of branches. Enable branch coverage in pyproject.toml and add a test extra. No fail_under yet: the point of this commit is an honest baseline. Five tests are marked xfail(strict=True). Each pins a defect found while writing the suite rather than encoding it as expected behaviour, so the suite fails again once the defect is fixed and the marker has to go. All five predate this branch, they come from the initial import of the repository. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
force-pushed
the
add-tests-and-coverage
branch
3 times, most recently
from
August 6, 2026 20:38
f4d1e4d to
6f6c9d8
Compare
insatomcat
force-pushed
the
add-tests-and-coverage
branch
2 times, most recently
from
August 6, 2026 21:05
8c3521c to
df32903
Compare
eroussy
requested changes
Sep 7, 2026
insatomcat
force-pushed
the
add-tests-and-coverage
branch
from
September 7, 2026 14:38
df32903 to
a82950f
Compare
The repository had no CI. Add a workflow that runs the suite on python 3.9 to 3.13, publishes the coverage table in the run summary, and checks that the wheel builds reproducibly. Coverage evidence does not depend on a third party: the OpenSSF criteria are self-asserted and only require a FLOSS tool able to measure them, which coverage.py is. The run summary is therefore enough on its own. Reproducible build: two builds with SOURCE_DATE_EPOCH pinned produce byte-identical wheels, without it they differ because setuptools stamps the archive with the source mtimes. The job builds the wheel twice and compares the SHA-256. It builds outside the work tree, since the project uses a flat layout and an output directory next to setup_ovs/ would be picked up as a second top-level package. Validation happens on pull requests, which build the simulated merge commit. main is built too, but minimally, one python version and no reproducible build job: SonarCloud needs an analysis of main as the reference for the new code comparison, and that analysis needs a coverage report. The scanner is given the version read from pyproject.toml. The project compares against the previous version to decide what counts as new code, and an analysis carrying no version leaves that period without a boundary: it falls back to the first analysis ever and the whole code base counts as new. vm_manager hit exactly that when it started publishing coverage, and its main gate turned red on pre-existing code. The actions published by GitHub use their major version tag. The Sonar action comes from a third party, whose tags can be moved by its publisher, so it is pinned to a full commit SHA. The CI toolchain is pinned in requirements-ci.txt. sonar-project.properties declares the coverage report path. It only takes effect once Automatic Analysis is turned off on the SonarCloud project, since that mode never runs the tests. The scanner step stays skipped while SONAR_TOKEN is unset. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
force-pushed
the
add-tests-and-coverage
branch
from
September 7, 2026 14:38
a82950f to
a7f13ad
Compare
Member
Author
|
Done, all seven switched to tags: checkout v6, setup-python v7, upload-artifact v7, download-artifact v8. I left the SHA on SonarSource/sonarqube-scan-action, since a third-party tag can be moved by its publisher and that job holds SONAR_TOKEN. |
|
eroussy
approved these changes
Sep 7, 2026
This was referenced Sep 7, 2026
insatomcat
added a commit
that referenced
this pull request
Sep 14, 2026
dpdk_interfaces and system_interfaces were locals of _check_port_configuration, which runs once per port. Both lists were therefore rebuilt empty on every call and the two "already used in another port" guards could never fire: the same NIC could be claimed by any number of ports without the check saying anything. Hoist the two lists into configuration_check and pass them down, so they accumulate across every port of every bridge. A NIC is claimed by a single port anywhere in the configuration, so the guard deliberately spans bridges and not just the current one. The defect dates back to 6c83aa0, "init repo from meta-seapath folder". It was covered by an xfail(strict=True) marker added in #22, now removed, and the tests cover the two-ports, two-bridges and system-interface cases. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
added a commit
that referenced
this pull request
Sep 14, 2026
The three come from 6c83aa0, "init repo from meta-seapath folder", and each was covered by an xfail(strict=True) marker added in #22. They sit in the same forty lines of _check_port_configuration and share the same range checkers, so they are fixed together. The tag range was never checked. The guard read "vlan" in port while the attribute ovs._create_bridges actually consumes is "tag", so any tag value went through untouched, and a configuration carrying "vlan" without "tag" raised KeyError on port["tag"] instead. Validate "tag". "vlan" is consumed nowhere and is now simply ignored. The "must be set if type is vxlan" branch was unreachable. It sat inside "if attribute in port" and then tested "attribute not in port", which cannot both hold. A vxlan port declaring neither key nor remote_ip passed the check and blew up later in ovs.py on port["remote_ip"]. The requirement is now tested on the port type instead of on the attribute, and the "ignored when type is not vxlan" warning moves to the matching else. The IANA VXLAN port 4789 was rejected. _attribute_is_a_port is documented as a TCP/UDP port check but enforced the VLAN tag range, 0 to 4,095, so no configuration could use the default VXLAN destination port. Split the two ranges: _attribute_is_a_port accepts 0 to 65,535, and the new _attribute_is_a_vlan_tag keeps 0 to 4,095 for tag and trunks. Both delegate to _attribute_is_in_range, so the integer check is written once. Requiring key and remote_ip on vxlan ports is the one behaviour change that can reject a configuration accepted before. Such a configuration never worked: it crashed in _create_bridges with a KeyError. It now fails during the check with a message naming the missing attribute. Also drop a third argument passed to the mac error message, which has only two placeholders. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
added a commit
that referenced
this pull request
Sep 14, 2026
run_command silently did nothing whenever a caller passed check explicitly. The "return subprocess.run(...)" line sat inside the "if 'check' not in kargs" branch, so the only path that reached subprocess was the one that also set the default. Passing check=False, the very argument the docstring says the helper accepts, returned None without running anything. Dedent the call, and the stdout suppression with it: silencing stdout outside DEBUG has nothing to do with how check was obtained, so it now applies on both paths as the docstring describes. No caller passes check today, which is why it went unnoticed since 6c83aa0, "init repo from meta-seapath folder". It was a trap for the next one. The xfail(strict=True) marker added in #22 is removed. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
added a commit
that referenced
this pull request
Sep 14, 2026
Two places in _create_bridges disagreed with what configuration_check accepts, so a configuration that passed the check crashed while being applied. remote_port is validated as an integer, and _create_bridges built its argument with "options:remote_port=" + port["remote_port"]. Every vxlan port carrying a remote_port therefore raised TypeError. Format the value instead. trunks is validated as an integer or an integer list, and _create_bridges iterated port["trunks"] directly, so a bare integer raised TypeError. Normalise it the same way check.py does before joining. Neither was caught by the tests added in #22 because test_ovs.py drives _create_bridges directly and happened to pass remote_port as a string, a value configuration_check rejects. The tests now use the types the check actually produces, and cover the scalar trunks case. Both defects date back to 6c83aa0, "init repo from meta-seapath folder", except the trunks list form, which arrived with the attribute. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
added a commit
that referenced
this pull request
Sep 14, 2026
Two metadata defects, left untouched by #22 because both depend on what the project is supported on rather than on what the code tolerates. requires-python claimed ">=3.6". The package cannot run on 3.6: check.py and ovs.py call subprocess.run with capture_output, an argument added in 3.7, so system_check and clear_ovs raise TypeError there. The real floor is at least 3.7, and 3.7 and 3.8 are both end of life and absent from the GitHub runners, so neither can be tested. Declare ">=3.9", the lowest version the CI matrix actually verifies. Every live deployment target is above it: Debian trixie ships 3.13, Yocto wrynose 3.14, and the oldest manifest still around, kirkstone, 3.10. license was a TOML table, which setuptools deprecates in favour of a plain SPDX expression. The build printed a SetuptoolsDeprecationWarning on every run announcing removal by 2027-02-18. Use the string form together with license-files, and raise the build-system requirement to setuptools 77, the release that introduced both. The metadata now says License-Expression: Apache-2.0 under Metadata-Version 2.4, and the LICENSE file is still shipped in the wheel. The build is warning-free and stays byte-for-byte reproducible under SOURCE_DATE_EPOCH. The setuptools bump is safe for the two maintained targets, which is what matters here: both Ansible roles install with --no-build-isolation, so build-system requires is not honoured on target and the setuptools already present is the one that counts. Debian trixie ships 78.1.1 and Yocto wrynose 82.0.1, both above 77. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
insatomcat
added a commit
that referenced
this pull request
Sep 14, 2026
Two metadata defects, left untouched by #22 because both depend on what the project is supported on rather than on what the code tolerates. requires-python claimed ">=3.6". The package cannot run on 3.6: check.py and ovs.py call subprocess.run with capture_output, an argument added in 3.7, so system_check and clear_ovs raise TypeError there. The real floor is at least 3.7, and 3.7 and 3.8 are both end of life and absent from the GitHub runners, so neither can be tested. Declare ">=3.9", the lowest version the CI matrix actually verifies. Every live deployment target is above it: Debian trixie ships 3.13, Yocto wrynose 3.14, and the oldest manifest still around, kirkstone, 3.10. license was a TOML table, which setuptools deprecates in favour of a plain SPDX expression. The build printed a SetuptoolsDeprecationWarning on every run announcing removal by 2027-02-18. Use the string form together with license-files, and raise the build-system requirement to setuptools 77, the release that introduced both. The metadata now says License-Expression: Apache-2.0 under Metadata-Version 2.4, and the LICENSE file is still shipped in the wheel. The build is warning-free and stays byte-for-byte reproducible under SOURCE_DATE_EPOCH. The setuptools bump is safe for the two maintained targets, which is what matters here: both Ansible roles install with --no-build-isolation, so build-system requires is not honoured on target and the setuptools already present is the one that counts. Debian trixie ships 78.1.1 and Yocto wrynose 82.0.1, both above 77. Signed-off-by: Florent Carli <florent.carli@rte-france.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



The repository had no test and no CI. The two OpenSSF gold coverage criteria (
test_statement_coverage90andtest_branch_coverage80) could therefore not be evaluated at all: they ask for a measured figure, not for a test suite that merely exists.What this brings
A pytest suite of 219 tests over the five modules, running in about 0.2 s. Everything that touches the system is mocked (
subprocess, sysfs,/proc, the network stack), so the suite needs neither root, nor OVS, nor a cluster, and runs on any machine.Measured result:
test_statement_coverage90test_branch_coverage80build_reproducibleSOURCE_DATE_EPOCHContents
tests/, atestextra, and branch coverage enabled inpyproject.tomlsonar-project.propertiesso the report is importedrequirements-ci.txt, a pinned toolchain used as a pip constraints file, so a CI run does not silently pick up a new release of pytest or build between two runs. Every pin supports Python 3.9 to 3.13. Actions are pinned to full commit SHAs for the same reason, with the major version kept in a trailing comment.setup_ovs.pyimportsyamlto read.yamland.ymlconfigurations, but nothing required it, so an installed package crashed on those files.Reproducible build
Measured both ways rather than assumed. Two builds with
SOURCE_DATE_EPOCHpinned produce byte-for-byte identical wheels. Without it they differ: setuptools stamps the archive with the source file mtimes, which change on every checkout. Thereproducible-buildjob therefore builds the wheel twice and compares the SHA-256 of the two archives.Bugs found while writing the tests, to be fixed in a follow-up
This pull request changes no production code.
git diff main..HEAD -- setup_ovs/is empty. The five defects below were not introduced here:git log -Lputs every one of them in6c83aa0, "init repo from meta-seapath folder", 2022-05-21. They have been there since the repository was created, over four years ago.Writing the tests did not create them, it executed those code paths for the first time. That is also why the coverage is not 100 %: the four uncovered statements in
check.pyare literally the dead code these defects produce, and no input can reach them.They are deliberately left unfixed here. Correcting them means changing the behaviour of validation code that has been running on SEAPATH clusters for four years, which deserves its own pull request and its own discussion rather than being buried in a test suite. Mixing the two would also make this diff impossible to review: right now it adds tests and touches nothing else.
None of them is encoded as expected behaviour either. Each is marked
xfail(strict=True)with its reason, so the day one is fixed the suite goes red and the marker has to be removed. The cleanup cannot be forgotten. They are listed intests/test_helpers.py:180andtests/test_check.pylines 324, 382, 427 and 520, andpytest -rxprints them with their reason on every run.helpers.run_commanddoes nothing when the caller passescheck=(helpers.py:60). Thereturn subprocess.run(...)sits inside theif "check" not in kargsbranch. No caller passeschecktoday, so it is latent, but it is a trap for the next one.tagrange is never validated (check.py:283). The guard isif "vlan" in portwhile the attribute actually consumed byovs._create_bridgesistag.check.py:270): it sits insideif attribute in portand then testsattribute not in port. A vxlan port with neitherkeynorremote_ippasses the check, then raisesKeyErrorfurther down inovs.py:239.check.py:111)._attribute_is_a_portis documented as a TCP/UDP port check but enforces the VLAN tag range, 0 to 4095. A TCP/UDP port goes up to 65535.check.py:165).dpdk_interfacesandsystem_interfacesare locals of_check_port_configuration, which runs once per port, so they are always empty.SonarCloud analysis
The project now runs CI-based analysis. Automatic Analysis has been turned off
and
SONAR_TOKENis configured, so the scanner step actually runs and thecoverage report is imported. Automatic Analysis could never have done it: it
runs neither the build nor the tests, and the Sonar documentation states
plainly that
"Code coverage information is not supported".The first real analysis confirms the figures measured locally:
line_coveragebranch_coveragecoverage(combined)The quality gate passes on its five conditions.
new_coverageis notevaluated, since
new_lines_to_coveris empty: this branch adds no line tosonar.sources.Worth knowing for later: the analysis token expires. When it does, the scanner
will fail with a
403and nothing will warn beforehand. That is exactly thestate vm_manager is in today, with a token issued in 2023.
Why the CI matrix does not start at Python 3.6
pyproject.tomldeclaresrequires-python = ">=3.6". That declaration is factually wrong: the package cannot run on 3.6.subprocess.run()is called withcapture_output=Truein two places,setup_ovs/check.py:18andsetup_ovs/ovs.py:76. That argument was added in Python 3.7. On 3.6 both calls raiseTypeError, which meanssystem_check()andclear_ovs()fail immediately. So the real floor is at least 3.7, whatever the metadata says. Python 3.6 itself has been end of life since December 2021.The matrix therefore runs 3.9 to 3.13, the versions available on current GitHub runners. 3.7 and 3.8 are also end of life and are no longer provided there, so testing the declared floor is not possible even if we wanted to.
requires-pythonis left untouched here, and so is the rest of the packaging metadata. Fixing the declared floor means deciding what the package is supported on, which depends on the SEAPATH deployment targets rather than on what the code happens to tolerate, and it belongs with a second metadata fix: the build warns thatlicense = { text = "Apache-2.0" }should become a plain SPDX string, which requires raisingsetuptoolsto 77 inbuild-systemand could break building on distributions that ship an older one. That warning is not fatal before 2027-02-18. Both go together in a dedicated packaging pull request, so this one keeps adding tests and touching nothing else.