paulrutter commented on PR #544:
URL: https://github.com/apache/felix-dev/pull/544#issuecomment-5676818309
### Proposal: wire `fileinstall` into GitHub CI so these unit tests actually
run
While reviewing this PR I noticed `fileinstall` is not covered by
`.github/workflows/maven-ci.yml`, so the new `ConfigInstallerTest` cases added
here never run in CI. Posting the change as a proposal rather than a separate
PR — feel free to fold it into this branch or take it separately.
Two things were needed.
**1. `mvn verify` cannot run on `fileinstall` under JDK 17+**
`fileinstall` still inherits `felix-parent` **6** (every module currently in
CI is on 8 or 9). Parent 6 binds `ianal-maven-plugin:1.0-alpha-1`, which dies
under strong encapsulation:
```
Failed to execute goal
org.codehaus.mojo:ianal-maven-plugin:1.0-alpha-1:verify-legal-files
Unable to make private java.io.File(java.lang.String,java.io.File)
accessible:
module java.base does not "opens java.io" to unnamed module
```
So the CI step uses `clean test` rather than the `clean verify` the other
modules use. Moving the module to a newer parent would be the better long-term
fix, but that is a bigger change than a CI tweak and would affect the released
bundle, so I left it alone and documented the reason in the workflow.
**2. `DirectoryWatcherTest.testInvalidTempDir` is stale and fails on Linux**
This one is a genuine pre-existing failure on `master`, unrelated to this
PR. FELIX-6794 (326327004c) replaced the hand-rolled temp-dir logic — which
read `System.getProperty("java.io.tmpdir")` fresh on every call — with
`Files.createTempDirectory()`. That uses `TempFileHelper.tmpdir`, a cached
static initialised on first use, so the test's
`System.setProperty("java.io.tmpdir", ...)` no longer influences the code under
test at all. Demonstrated standalone on JDK 21:
```
prop now = /nonexistent/nope
created = /tmp/fileinstall-10512125911307238276
```
The rewrite drives the still-reachable failure path instead: configure
`felix.fileinstall.tmpdir` to a directory that cannot be created, and assert
the constructor rejects it. The parent is a regular file rather than a
write-protected directory, so it fails deterministically for any user —
including root, which matters if anyone runs the build in a container.
I mutation-checked the rewritten test: it passes with `prepareDir`'s
validation intact and fails when that validation is stubbed out, so it is not
passing vacuously.
The extra `getServiceReference(LogService.class)` stub in `setUp` is needed
because `Util.getLogService()` calls the `Class`-typed overload, while the
fixture only stubbed the `String` one — previously unnoticed since no test
reached an `ERROR`-level log.
### Verification
Run on Linux (CI runs `ubuntu-latest`), `mvn clean test`:
| JDK | Result |
| --- | --- |
| 21 | `Tests run: 32, Failures: 0, Errors: 0` — BUILD SUCCESS |
| 25 | `Tests run: 32, Failures: 0, Errors: 0` — BUILD SUCCESS |
JDK 25 stands in as an upper bound for the matrix's 23. I could not verify
**JDK 17** locally (not packaged for my distro) — worth a glance at the first
CI run, though `source`/`target` 8 on 17 is the least risky of the three.
Note for anyone reproducing on Windows: 5 further tests in
`DirectoryWatcherTest`/`ConfigInstallerTest` fail there because they build
expected URIs by string concatenation (`"file:" + absolutePath`, which yields
`file:C:\...` instead of `file:/C:/...`). Those are Windows-only and I left
them untouched, since CI is Linux.
### Diff
```diff
diff --git a/.github/workflows/maven-ci.yml b/.github/workflows/maven-ci.yml
index 3658e01105..cf36e0bc76 100644
--- a/.github/workflows/maven-ci.yml
+++ b/.github/workflows/maven-ci.yml
@@ -11,6 +11,7 @@ on:
- 'log/**'
- 'webconsole/**'
- 'framework/**'
+ - 'fileinstall/**'
pull_request:
branches: [ "master" ]
paths:
@@ -22,6 +23,7 @@ on:
- 'log/**'
- 'framework/**'
- 'gogo/**'
+ - 'fileinstall/**'
permissions: {}
@@ -65,6 +67,8 @@ jobs:
- 'framework/**'
gogo:
- 'gogo/**'
+ fileinstall:
+ - 'fileinstall/**'
- name: Felix SCR
if: steps.changes.outputs.scr == 'true'
@@ -93,6 +97,13 @@ jobs:
- name: Felix Gogo Shell
if: steps.changes.outputs.gogo == 'true'
run: mvn -B -V -Dstyle.color=always --file gogo/pom.xml clean verify
+ # fileinstall still inherits felix-parent 6, which binds
ianal-maven-plugin
+ # 1.0-alpha-1. That plugin fails on JDK 17+ ("module java.base does not
opens
+ # java.io"), so the 'verify' phase cannot run here. Stick to 'test'
until the
+ # module is moved to a newer parent.
+ - name: Felix File Install
+ if: steps.changes.outputs.fileinstall == 'true'
+ run: mvn -B -V -Dstyle.color=always --file fileinstall/pom.xml clean
test
- name: Upload Test Results
if: always()
uses:
actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
diff --git
a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
index 6769c4d3d5..49d02467fd 100644
---
a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
+++
b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
@@ -74,6 +74,8 @@ public class DirectoryWatcherTest extends TestCase
.andStubReturn(null);
EasyMock.expect(mockBundleContext.getServiceReference(LogService.class.getName()))
.andStubReturn(null);
+
EasyMock.expect(mockBundleContext.getServiceReference(LogService.class))
+ .andStubReturn(null);
EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
EasyMock.expect(mockBundleContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION)).andReturn(mockSysBundle).anyTimes();
EasyMock.expect(mockSysBundle.getState()).andReturn(Bundle.ACTIVE).anyTimes();
@@ -237,39 +239,27 @@ public class DirectoryWatcherTest extends TestCase
public void testInvalidTempDir() throws Exception
{
- String oldTmpDir = System.getProperty("java.io.tmpdir");
-
- try
+ // Point felix.fileinstall.tmpdir at a path whose parent is a
regular file, so that
+ // creating the directory is bound to fail for any user running the
build (root
+ // included). Overriding java.io.tmpdir would not work here:
Files.createTempDirectory
+ // caches that property on first use, so a later System.setProperty
has no effect.
+ File blocker = new File( "target/not-a-directory" );
+ blocker.getParentFile().mkdirs();
+ blocker.delete();
+ assertTrue( "Unable to create test fixture file " + blocker,
blocker.createNewFile() );
+
+ props.put( DirectoryWatcher.TMPDIR, new File( blocker, "tmp"
).getAbsolutePath() );
+
+ EasyMock.replay(mockBundleContext, mockBundle, mockSysBundle,
mockStartLevel);
+
+ try
{
- File parent = new File("target/tmp");
- parent.mkdirs();
- parent.setWritable(false, false);
- File tmp = new File(parent, "tmp");
- System.setProperty("java.io.tmpdir", tmp.toString());
-
- mockBundleContext.addBundleListener((BundleListener)
org.easymock.EasyMock.anyObject());
- EasyMock.expect(mockBundleContext.createFilter((String)
EasyMock.anyObject()))
- .andReturn(null);
-
- BundleRevision mockBundleRevision =
EasyMock.createNiceMock(BundleRevision.class);
-
EasyMock.expect(mockBundle.adapt(BundleRevision.class)).andReturn(mockBundleRevision);
- EasyMock.expect(mockBundleRevision.getTypes())
- .andReturn(BundleRevision.TYPE_FRAGMENT);
- EasyMock.replay(mockBundleContext, mockBundle,
mockBundleRevision, mockSysBundle, mockStartLevel);
-
- try
- {
- dw = new DirectoryWatcher( new FileInstall(), props,
mockBundleContext );
- fail("Expected an IllegalStateException");
- }
- catch (IllegalStateException e)
- {
- // expected
- }
+ dw = new DirectoryWatcher( new FileInstall(), props,
mockBundleContext );
+ fail("Expected a RuntimeException for a temp directory that
cannot be created");
}
- finally
+ catch (RuntimeException e)
{
- System.setProperty("java.io.tmpdir", oldTmpDir);
+ // expected
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
