Skip to content

STDO-124: Fix NPE and test-isolation failures #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
alextwigkit wants to merge 1 commit into
base: master
Choose a base branch
from
+54 −6
Draft

STDO-124: Fix NPE and test-isolation failures #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions docs/STDO-124-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
**RESUME STATE** *(top of the file; rewritten in place, never appended to; keep under 12 lines)*
- **Reconciled at**: this branch's HEAD, the P4 build-verification fix commit.
- **Authoritative**: `PLAN.md` in `lucidworks/tbe-pitches`, branch `STDO-124-bet`, for stage
sequencing, the done-condition, the stage graph and every measured claim. This file exists only
so `/team-studios:status` and the compaction resume hook have something to find in this repo.
- **Next**: this repo's release cut (an actual version tag) is a human action, not part of P4's
done-condition.

**Decisions** *(append-only)*
- **This file is a pointer, not a fork of the plan.** Full decision log: `tbe-pitches`'s
`decision-log.md` on `STDO-124-bet`.
- **Two real defects fixed, not worked around**, to get a clean `mvn clean package` from an empty
local repository: a `NullPointerException` in `PropertiesLoader.readFolder` on a `null`
`listFiles()` result, and four test failures caused by a process-wide `Fig` singleton left
mutated by `FigUtilsTest` with no teardown.

**Wiki candidates** *(append during the work, not at the end)*
- (empty)

---

resume-state template r3-tamarind
7 changes: 7 additions & 0 deletions fig-core/src/main/java/twigkit/fig/loader/PropertiesLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ public boolean accept(File file, String s) {
};

File[] files = folder.listFiles(filter);
if (files == null) {
logger.error("Unable to list files in folder (not a directory, or an I/O error occurred): {}", folder);
files = new File[0];
}

Arrays.sort(files, new Comparator<File>() {
public int compare(File file, File file1) {
Expand Down Expand Up @@ -123,6 +127,9 @@ public boolean accept(File file) {
};

File[] nestedFolders = folder.listFiles(folderFilter);
if (nestedFolders == null) {
nestedFolders = new File[0];
}
Comment on lines 93 to +132

@alextwigkit alextwigkit Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2 — high] New null-guard branches for folder.listFiles() have no regression test anywhere in the repo

Neither new null-guard branch (lines 93-97 for the file-listing call, lines 129-132 for the nested-folder-listing call) is exercised by any test. fig-core/pom.xml has no Mockito/EasyMock/PowerMock or similar mocking dependency, and every existing test in PropertiesLoaderTest.java/MergedPropertiesLoaderTest.java passes an already-confirmed real directory into readFolder, so listFiles() never returns null in the current test suite.

Tip

Suggested: add a unit test that points a PropertiesLoader at a File that is not a directory (so listFiles() returns null per the java.io.File contract) and assert readFolder/load completes without throwing. This is the cheapest way to hit the guard without adding a mocking library dependency.

💡 Copy this prompt to fix with Claude Code 
In twigkit/fig on the branch for PR #33, fix this issue:

File: fig-core/src/main/java/twigkit/fig/loader/PropertiesLoader.java
Line(s): 93-132

Problem: New null-guard branches for folder.listFiles() have no regression test anywhere in the repo
### [P2 — high] New null-guard branches for folder.listFiles() have no regression test anywhere in the repo

Neither new null-guard branch (lines 93-97 for the file-listing call, lines 129-132 for the nested-folder-listing call) is exercised by any test. fig-core/pom.xml has no Mockito/EasyMock/PowerMock or similar mocking dependency, and every existing test in PropertiesLoaderTest.java/MergedPropertiesLoaderTest.java passes an already-confirmed real directory into readFolder, so listFiles() never returns null in the current test suite.

> [!TIP]
> Suggested: add a unit test that points a PropertiesLoader at a File that is not a directory (so listFiles() returns null per the java.io.File contract) and assert readFolder/load completes without throwing. This is the cheapest way to hit the guard without adding a mocking library dependency.

After fixing, respond to the review comment on PR #33 in twigkit/fig
confirming the fix. Finding: "New null-guard branches for folder.listFiles() have no regression test anywhere in the repo" in fig-core/src/main/java/twigkit/fig/loader/PropertiesLoader.java.

for (File nestedFolder : nestedFolders) {
readFolder(fig, nestedFolder);
}
Expand Down
31 changes: 25 additions & 6 deletions fig-core/src/test/java/twigkit.fig/util/FigUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package twigkit.fig.util;

import org.junit.After;
import org.junit.Test;
import twigkit.fig.Config;
import twigkit.fig.Fig;
Expand All @@ -13,9 +14,27 @@
*/
public class FigUtilsTest {

/**
* {@link Fig#getInstance(twigkit.fig.loader.Loader...)} returns a process-wide singleton
* keyed on the loader(s) used. {@link FigUtils#merge(Fig, Fig)} mutates its first
* argument in place, so merging into the singleton for "confs" here would otherwise
* permanently leave that shared instance with merged-in data for the rest of the test
* run, corrupting unrelated tests (e.g. in {@code MergedPropertiesLoaderTest}) that
* expect to see the pristine "confs" configuration. Reloading after each test restores
* the singleton to its original, unmerged state.
*/
private Fig primary;

@After
public void restoreSharedPrimaryFig() {
if (primary != null) {
primary.reload();
}
}
Comment on lines +17 to +33

@alextwigkit alextwigkit Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3 — medium] Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets

By inspection this teardown is correct: Fig.getInstance keys its singleton map on loader path, so FigUtilsTest and MergedPropertiesLoaderTest share the same "confs" Fig instance, and Fig.reload() rebuilds configs from scratch, discarding FigUtils.merge()'s in-place mutation. However, no test in the repo mechanically forces FigUtilsTest to run immediately before MergedPropertiesLoaderTest and asserts the latter still observes pristine "confs" data — the regression this PR targets is guarded only by the PR description's manual "isolated before/after" verification, not by an automated check.

Tip

Suggested: add a combined-suite test (or @FixMethodOrder) that runs a FigUtilsTest-style merge against the shared "confs" singleton, then re-asserts the pristine values a MergedPropertiesLoaderTest case depends on — converting the current one-time manual verification into a standing regression check.

💡 Copy this prompt to fix with Claude Code 
In twigkit/fig on the branch for PR #33, fix this issue:

File: fig-core/src/test/java/twigkit.fig/util/FigUtilsTest.java
Line(s): 17-33

Problem: Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets
### [P3 — medium] Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets

By inspection this teardown is correct: Fig.getInstance keys its singleton map on loader path, so FigUtilsTest and MergedPropertiesLoaderTest share the same "confs" Fig instance, and Fig.reload() rebuilds configs from scratch, discarding FigUtils.merge()'s in-place mutation. However, no test in the repo mechanically forces FigUtilsTest to run immediately before MergedPropertiesLoaderTest and asserts the latter still observes pristine "confs" data — the regression this PR targets is guarded only by the PR description's manual "isolated before/after" verification, not by an automated check.

> [!TIP]
> Suggested: add a combined-suite test (or @FixMethodOrder) that runs a FigUtilsTest-style merge against the shared "confs" singleton, then re-asserts the pristine values a MergedPropertiesLoaderTest case depends on — converting the current one-time manual verification into a standing regression check.

After fixing, respond to the review comment on PR #33 in twigkit/fig
confirming the fix. Finding: "Singleton-isolation fix has no automated test proving it prevents the cross-class failure it targets" in fig-core/src/test/java/twigkit.fig/util/FigUtilsTest.java.


@Test
public void testExistingConfigPropertiesAreLeftUnchanged() {
Fig primary = Fig.getInstance(new PropertiesLoader("confs"));
primary = Fig.getInstance(new PropertiesLoader("confs"));
Fig secondary = Fig.getInstance(new PropertiesLoader("confs_dev"));

String originalRoot1KeyValue = primary.find("root").value("root-1-key").as_string();
Expand All @@ -38,7 +57,7 @@ public void testExistingConfigPropertiesAreLeftUnchanged() {

@Test
public void testExistingConfigsAreUpdatedWithNewPropertyValues() {
Fig primary = Fig.getInstance(new PropertiesLoader("confs"));
primary = Fig.getInstance(new PropertiesLoader("confs"));
Fig secondary = Fig.getInstance(new PropertiesLoader("confs_dev"));

FigUtils.merge(primary, secondary);
Expand All @@ -55,7 +74,7 @@ public void testExistingConfigsAreUpdatedWithNewPropertyValues() {

@Test
public void testExistingConfigsAreUpdatedWithNewProperties() {
Fig primary = Fig.getInstance(new PropertiesLoader("confs"));
primary = Fig.getInstance(new PropertiesLoader("confs"));
Fig secondary = Fig.getInstance(new PropertiesLoader("confs_dev"));

FigUtils.merge(primary, secondary);
Expand All @@ -70,7 +89,7 @@ public void testExistingConfigsAreUpdatedWithNewProperties() {

@Test
public void testExistingConfigsAreUpdatedWithNewExtensions() {
Fig primary = Fig.getInstance(new PropertiesLoader("confs"));
primary = Fig.getInstance(new PropertiesLoader("confs"));
Fig secondary = Fig.getInstance(new PropertiesLoader("confs_dev"));

FigUtils.merge(primary, secondary);
Expand All @@ -81,7 +100,7 @@ public void testExistingConfigsAreUpdatedWithNewExtensions() {

@Test
public void testNewConfigsCanBeAdded() {
Fig primary = Fig.getInstance(new PropertiesLoader("confs"));
primary = Fig.getInstance(new PropertiesLoader("confs"));
Fig secondary = Fig.getInstance(new PropertiesLoader("confs_dev"));

FigUtils.merge(primary, secondary);
Expand All @@ -92,7 +111,7 @@ public void testNewConfigsCanBeAdded() {

@Test
public void testChildConfigPropertyValuesCanBeUpdated() {
Fig primary = Fig.getInstance(new PropertiesLoader("confs"));
primary = Fig.getInstance(new PropertiesLoader("confs"));
Fig secondary = Fig.getInstance(new PropertiesLoader("confs_dev"));

FigUtils.merge(primary, secondary);
Expand Down