From 6ab9aca0b767cc3a1fae86268ad990a3fddf769e Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 23 Jul 2026 11:06:10 -0300 Subject: [PATCH] fix(xcat-core): split the source-only unit tests from the MN integration tests Three of the files under xCAT-test/unit are not unit tests. They need an installed management node rather than a checkout: copycds_packages_integrity.t wants an /install populated by a real copycds, dhcp_kea_config_validation.t wants a kea-dhcp4 binary that can read the config it generates, and dhcp_kea_control_agent_smoke.t wants live kea-dhcp4 and kea-ctrl-agent daemons running as root. On a GitHub runner none of that exists, so all three plan skip_all. They were the only three files skipping in the pull request run, which is not a coincidence -- the skip is the symptom of them being filed in the wrong place. A skipped test reports neither pass nor fail, so leaving them mixed in with the unit tests trains the reader to scroll past skips in a directory where a skip should mean something is wrong. Move them to xCAT-test/integration, ship that directory alongside unit in both the rpm and the deb, and drive it from a new xcattest testcase that proves the installed copy on an MN. The case is deliberately not labelled ci_test: the pull request workflow has no management node and must not pick it up. check:rc==0 is the right gate for it -- prove exits non-zero on a real failure, exits 0 when a test legitimately skips on a node without Kea, and exits 2 if the directory is missing entirely, so a packaging regression still fails the case. Add a README.md to each directory recording which side of the line a new test belongs on and how each suite is run. xCAT-test/unit is now 44 files and 802 assertions with no skips at all; the assertion count is unchanged, confirming the three moved files were contributing nothing but skips. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- .../autotest/testcase/integration/cases0 | 7 +++ xCAT-test/debian/install | 1 + xCAT-test/integration/README.md | 57 +++++++++++++++++++ .../copycds_packages_integrity.t | 0 .../dhcp_kea_config_validation.t | 0 .../dhcp_kea_control_agent_smoke.t | 0 xCAT-test/unit/README.md | 56 ++++++++++++++++++ xCAT-test/xCAT-test.spec | 1 + 8 files changed, 122 insertions(+) create mode 100644 xCAT-test/autotest/testcase/integration/cases0 create mode 100644 xCAT-test/integration/README.md rename xCAT-test/{unit => integration}/copycds_packages_integrity.t (100%) rename xCAT-test/{unit => integration}/dhcp_kea_config_validation.t (100%) rename xCAT-test/{unit => integration}/dhcp_kea_control_agent_smoke.t (100%) create mode 100644 xCAT-test/unit/README.md diff --git a/xCAT-test/autotest/testcase/integration/cases0 b/xCAT-test/autotest/testcase/integration/cases0 new file mode 100644 index 000000000..06773a075 --- /dev/null +++ b/xCAT-test/autotest/testcase/integration/cases0 @@ -0,0 +1,7 @@ +start:integration_tests +description:Run the xCAT-test integration tests (xCAT-test/integration) against the installed management node +os:Linux +label:mn_only,integration +cmd:prove -I/opt/xcat/lib/perl -I/opt/xcat/lib/perl/xCAT -r /opt/xcat/share/xcat/tools/autotest/integration +check:rc==0 +end diff --git a/xCAT-test/debian/install b/xCAT-test/debian/install index 1a4c56a89..8601c376f 100644 --- a/xCAT-test/debian/install +++ b/xCAT-test/debian/install @@ -4,3 +4,4 @@ share/man/man1/* opt/xcat/share/man/man1 share/doc/man1/* opt/xcat/share/doc/man1 autotest opt/xcat/share/xcat/tools unit opt/xcat/share/xcat/tools/autotest +integration opt/xcat/share/xcat/tools/autotest diff --git a/xCAT-test/integration/README.md b/xCAT-test/integration/README.md new file mode 100644 index 000000000..1dd6df4bc --- /dev/null +++ b/xCAT-test/integration/README.md @@ -0,0 +1,57 @@ +# xCAT-test/integration + +Integration tests. These run against an **installed management node** -- they need a +real xCAT installation, and depending on the test a populated `/install`, a service +binary they can execute, or a live daemon. + +They are *not* run by the GitHub Actions pull request workflow, which has no +management node. They are driven by `xcattest` through the testcase in +`../autotest/testcase/integration/`, which proves the copy installed by the +`xcat-test` package: + +``` +prove -I/opt/xcat/lib/perl -I/opt/xcat/lib/perl/xCAT \ + -r /opt/xcat/share/xcat/tools/autotest/integration +``` + +Run the case on an MN with: + +``` +xcattest -f -t integration_tests +``` + +Note the `-I` flags: unlike the unit tests these run from the installed location, so +they pick up xCAT modules from `/opt/xcat/lib/perl` rather than from a source tree. + +## What belongs here + +A test belongs in `integration/` when it needs something the checkout cannot provide: + +| Test | Requires | +| --- | --- | +| `copycds_packages_integrity.t` | `/install` populated by a real `copycds` | +| `dhcp_kea_config_validation.t` | a `kea-dhcp4` binary that can read the generated config | +| `dhcp_kea_control_agent_smoke.t` | live `kea-dhcp4` and `kea-ctrl-agent`, root, and the Kea host-commands hook | + +## Environment guards + +Tests here still guard with `plan skip_all` so the case does not fail on a node that +legitimately lacks the dependency -- an MN with no Kea installed should skip the Kea +tests, not go red. A skip in this directory is therefore expected and normal, which is +precisely why these tests do not belong alongside the unit tests. + +`dhcp_kea_control_agent_smoke.t` is opt-in on top of that: + +```perl +plan skip_all => 'set XCAT_KEA_LIVE_SMOKE=1 to run live Kea daemon smoke test' + unless $ENV{XCAT_KEA_LIVE_SMOKE}; +``` + +It starts real Kea daemons, so it stays off unless asked for. Do not enable it on a +node whose DHCP service is in use. + +## What does not belong here + +Anything that only needs the checkout. Those go in [`../unit`](../unit/README.md) and +run on every pull request, which is much faster feedback than waiting for a cluster +test. diff --git a/xCAT-test/unit/copycds_packages_integrity.t b/xCAT-test/integration/copycds_packages_integrity.t similarity index 100% rename from xCAT-test/unit/copycds_packages_integrity.t rename to xCAT-test/integration/copycds_packages_integrity.t diff --git a/xCAT-test/unit/dhcp_kea_config_validation.t b/xCAT-test/integration/dhcp_kea_config_validation.t similarity index 100% rename from xCAT-test/unit/dhcp_kea_config_validation.t rename to xCAT-test/integration/dhcp_kea_config_validation.t diff --git a/xCAT-test/unit/dhcp_kea_control_agent_smoke.t b/xCAT-test/integration/dhcp_kea_control_agent_smoke.t similarity index 100% rename from xCAT-test/unit/dhcp_kea_control_agent_smoke.t rename to xCAT-test/integration/dhcp_kea_control_agent_smoke.t diff --git a/xCAT-test/unit/README.md b/xCAT-test/unit/README.md new file mode 100644 index 000000000..739d7c3d9 --- /dev/null +++ b/xCAT-test/unit/README.md @@ -0,0 +1,56 @@ +# xCAT-test/unit + +Unit tests. These run against the **source tree only** -- no xCAT installation, no +running daemons, no management node. + +They are executed on every pull request by the `xcat_test` GitHub Actions workflow, +which calls `run_unit_tests()` in `github_action_xcat_test.pl`: + +``` +prove -r xCAT-test/unit +``` + +You can run exactly the same thing from a clean checkout: + +``` +cd +prove -r xCAT-test/unit +``` + +## What belongs here + +A test belongs in `unit/` when everything it needs is in the checkout: plugin and +library sources, kickstart/preseed/subiquity templates, postscripts, packaging +metadata. Such a test asserts on rendered output or module logic and reaches the +repository root through `FindBin`: + +```perl +use FindBin; +use lib "$FindBin::Bin/../../perl-xCAT"; +use lib "$FindBin::Bin/../../xCAT-server/lib/perl"; +``` + +Because of those `FindBin` paths the tests only work from a source tree. The copy +installed under `/opt/xcat/share/xcat/tools/autotest/unit` is not a substitute -- +`../..` resolves to `/opt/xcat/share/xcat/tools` there and the tests die or silently +skip. The CI takes a copy of the checkout before the build for this reason; see +`preserve_source_tree()`. + +## What does not belong here + +Anything that needs an installed xCAT, a populated `/install`, a real service binary +or a live daemon. Those go in [`../integration`](../integration/README.md) and run on +a management node through `xcattest`. + +The distinction matters because a test that needs an absent environment does not fail +-- it calls `plan skip_all` and reports as skipped. A handful of those in a suite of +several hundred assertions is easy to stop reading. Keeping the two kinds in separate +directories means a skip in `unit/` is a real signal rather than routine noise. + +Guarding on a *source* file, on the other hand, is fine and common here: + +```perl +plan skip_all => "compute.subiquity.tmpl not found" unless -f $tmpl_path; +``` + +That guard never fires when the tree is intact. diff --git a/xCAT-test/xCAT-test.spec b/xCAT-test/xCAT-test.spec index d4f759221..aafb56b6d 100644 --- a/xCAT-test/xCAT-test.spec +++ b/xCAT-test/xCAT-test.spec @@ -63,6 +63,7 @@ chmod 644 $RPM_BUILD_ROOT/%{prefix}/share/doc/man1/* cp -r autotest $RPM_BUILD_ROOT/%{prefix}/share/xcat/tools cp -r unit $RPM_BUILD_ROOT/%{prefix}/share/xcat/tools/autotest +cp -r integration $RPM_BUILD_ROOT/%{prefix}/share/xcat/tools/autotest %clean