GitHub ci extend#3842
Conversation
|
This looks broadly nice I wish this had happened before I was cleaning them up for jdk 25 :) |
|
I am impressed. |
|
I'm not personally a fan of having to review big changes, especially AI-assisted. GitHub actions are something that's quite easy to get wrong in very subtle, and insecure ways. I can't review everything that this PR does, so I've run the tool zizmor to audit some common issues with GitHub actions. Here's some of the most important ones (see the linked audit rules for a more complete explanation):
But there's more, here's the complete output of zizmor outputI recommend setting up the zizmor action so that those kind of issues are catched when modifying those jobs. In the EPNix repository "Actions" settings, I've also restricted which actions can run, to limit which transitive action dependencies can be run in our CI |
|
Zizmor looks pretty cool. I'll fix all the warnings on Monday, unless someone wants to take over. |
|
https://depmedicdev-byte.github.io/compare/ci-doctor-vs-zizmor.html The workflow - running |
|
to prevent this PR from becoming any bigger, added zizmor in a separate PR #3843 |
kasemir
left a comment
There was a problem hiding this comment.
Source changes look benign. Can't comment on the GitHub actions
Travis ran on years-EOL Ubuntu xenial and was effectively dead. Its headless TestFX MAVEN_OPTS (xvfb + glass robot + software prism) are ported into the new _test.yml reusable workflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
48e2b81 to
464baa5
Compare
Replace the old build.yml / build_latest.yml / four *-docker-image.yml setup
with reusable workflows invoked by thin, event-specific callers:
Reusable (workflow_call):
- _build, _test (unit + TestFX UI under xvfb), _integration-test
(Elasticsearch service container), _docker-image (build + publish to ghcr).
Event callers:
- branch.yml (push, non-master): fast build-only compile check.
- pr.yml (pull_request): build gate -> test + integration-test; covers
same-repo and fork PRs read-only (plain pull_request, no secrets).
- merge.yml (push master): cross-platform matrix build + test +
integration-test, and publishes images for changed services
(native git-diff detection).
- release.yml (push tag): publishes all service images.
- report.yml (workflow_run): publishes JUnit results as a check in the
base-repo context, so fork PRs get a check too.
Tests run as a required gate: a failure fails the job.
Also repoint the README CI status badge at merge.yml; the build.yml it
referenced is removed here.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add Dependabot (github-actions ecosystem) to auto-PR action-version bumps, and update CI_VERSIONS.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The *UI TestFX tests still extended the JUnit 4 base org.testfx.framework.junit.ApplicationTest while their @test methods were migrated to JUnit Jupiter (commit 9f87c08). Under the Jupiter engine the JUnit 4 lifecycle that invokes start(Stage) never runs, so the scene-graph fields are null and every test NPEs. They had been dormant since they were renamed to be skipped in CI; _test.yml's ui-tests profile re-enables them. Switch to the JUnit 5 TestFX base (org.testfx.framework.junit5.ApplicationTest) and align testfx to 4.0.18, matching app/email/ui. No other test changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to UI fork
CI test-gate fixes for the new integration-test and ui-test jobs.
integration-test: two problems kept the save-and-restore Elasticsearch-backed
*IT tests from passing.
- -Dspring.profiles.active=IT was forced on the whole reactor, including the
surefire unit-test phase, which excluded the @Profile("!IT") mock configs and
broke ~142 save-and-restore unit tests (NoSuchBeanDefinitionException:
NodeDAO). Move the IT profile onto the failsafe *IT fork only via
systemPropertyVariables in the module pom, drop the global -D so surefire keeps
the default (mock) profile, and build just the save-and-restore module and its
deps (-pl services/save-and-restore -am) so the job runs only the
Elasticsearch-backed *IT tests.
- The executable-jar profile's spring-boot:repackage runs at package and
replaces the module's main artifact with a fat jar (classes relocated under
BOOT-INF/classes/). Failsafe then ran against that repackaged artifact and
couldn't resolve the flat main classes the in-process @SpringBootTest ITs
field-inject (NoClassDefFoundError on ConfigurationDataRepository,
ElasticsearchTreeRepository, ComparisonController). Pass -Dskip-executable-jar
to deactivate the !skip-executable-jar profile so repackage is skipped and the
thin jar stays as the main artifact; the executable jar is built separately by
the docker-image job.
ui-tests (TestFX): the headless/testfx/prism properties were only in MAVEN_OPTS,
which reaches the launching Maven JVM but not the forked failsafe JVM, so
ListSelectionUI's java.awt.headless skip guard never fired (its leaked modal
dialog also broke DockPaneTestUI.TestContextMenu). Set them as failsafe
systemPropertyVariables in the ui-tests profile so the guard works and the dock
tests drive a glass robot under xvfb.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at-end Previously a single _test.yml job ran unit (surefire) and TestFX UI (failsafe *UI) tests together, and the fail-fast reactor skipped every later module once one module's tests failed. - Add _unit-test.yml: surefire tests across the reactor with --fail-at-end, so every independent module is tested even when another fails. - Add _ui-test.yml: the *UI TestFX tests only, under xvfb. The ui-tests profile now skips surefire (plugin-config level, so failsafe still runs), making -P ui-tests mean 'UI tests only'. - Rewire pr.yml and merge.yml to call both reusable workflows; remove _test.yml. - report.yml already globs *test-reports, so it picks up the unit-test-reports, ui-test-reports and integration-test-reports artifacts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Some unit tests (e.g. those using javafx.scene.paint.Color) require the JavaFX toolkit to be initialized, which fails in a pure headless environment without xvfb or specific prism properties. - Rename 11 unit tests that import javafx.* to *FXTest.java. - Exclude **/*FXTest.java from the default surefire execution in pom.xml. - Include **/*FXTest.java in the ui-tests and all-tests profiles via failsafe. - Remove xvfb-run and graphics properties from _unit-test.yml, making it a truly headless job. - These *FXTest tests will now run in the _ui-test.yml job under xvfb. Co-Authored-By: Gemini-Cli <218195315+gemini-cli@users.noreply.github.com>
The integration tests shared one cached Spring context, one JVM fork and one fixed set of Elasticsearch indices. Root-node/index creation was gated by a static esInitialized flag (once per JVM), while ElasticsearchTreeRepository- TestIT's deleteAll() wiped the whole tree index including the root. The root was then never recreated, so a later class in the fork (e.g. ComparisonControllerTestIT) failed with NodeNotFoundException on the root. Drop the static esInitialized flag so each ElasticConfig/context initializes its own indices and root (both helpers are already idempotent). Add an AbstractElasticsearchIT base class that gives every IT class its own random index names via @DynamicPropertySource, drops them in @afterall, and uses @DirtiesContext(AFTER_CLASS) so each class rebuilds its context (the context cache key is derived from the @DynamicPropertySource method, not its random values, so without eviction all subclasses would share one context). The four IT classes now extend it and no longer pollute one another. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add it as a lint github action Add zizmor Add TODO to enable end of file fixer and whitespace fixer in a follow up
persist credentials false pass inputs via env
prism.text, testfx.setup.timeout, and testfx.headless were passed via MAVEN_OPTS, which only reaches the launching Maven JVM and never the forked test JVM where the UI tests run. Move them onto failsafe's systemPropertyVariables alongside the three properties already fixed the same way, then drop the now-redundant maven-opts plumbing from the test workflows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
464baa5 to
a81c2eb
Compare
|
|
@minijackson Added zizmor via pre-commit, and fixed all the suggestions. @georgweiss I altered Save and restore ElasticConfig, removed the initialized check to make testing easier. Seemed like it isn't necessary as there is an exists check on the elasticIndexValidation. |



Add testing to github ci.
Open questions:
Checklist
Testing:
Documentation: