Skip to content

fix: avoid sharing List values set across samples - #1349

Closed
shin4141 wants to merge 4 commits into
naver:mainfrom
shin4141:codex/v35-fixture-monkey-list-isolation
Closed

shin4141 wants to merge 4 commits into
naver:mainfrom
shin4141:codex/v35-fixture-monkey-list-isolation

Conversation

@shin4141

Copy link
Copy Markdown

Summary

  • Produce a fresh ArrayList for each sample when a List value reaches the set-decomposition fast path.
  • Preserve the existing container size and wildcard handling.
  • Add regression tests for sample independence and for a fixed-size input list producing a mutable sample.

Reproduction

On main d6e285af401f06a3934e2caddfdad052000276ce, both new tests fail: samples share the supplied list, and removing an item from a sample created from Arrays.asList throws UnsupportedOperationException. This addresses the List examples in #1348.

Tests

  • ./gradlew :fixture-monkey:test --tests com.navercorp.fixturemonkey.test.SetCollectionIsolationTest --no-daemon — 2/2 pass after the fix (2/2 failed before).
  • ./gradlew :fixture-monkey:test --no-daemon — 1,225/1,225 pass.

Scope

The change covers List values whose declared type accepts ArrayList. Other concrete List types and nested mutable elements are outside this patch.

@CLAassistant

CLAassistant commented Sep 30, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment on lines +308 to +310
if (earlyValue instanceof List && currentRawType.isAssignableFrom(ArrayList.class)) {
List<?> list = (List<?>)earlyValue;
earlyValue = CombinableArbitrary.from(() -> new ArrayList<>(list));

@seongahjo seongahjo Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@shin4141
Sorry for the late response.
I think this issue could occur in other concrete Collection classes, such as LinkedList, HashSet etc.

Can you suggest a more generic way of dealing with this value-sharing issue?

@seongahjo

Copy link
Copy Markdown
Contributor

@shin4141
Thank you for the PR and for following up on the review.

I looked into handling this by copying the value, as this PR does, and found it hard to make it work for nested structures such as List<List<String>>. Copying the outer collection still shares the inner ones, and covering every nesting and container type at the value level would need more and more type-specific branches.

The sharing actually comes from the assembly step: a set value was returned as is instead of being decomposed and rebuilt for each sample. Fixing it there takes some knowledge of how values are decomposed and matched to the node tree internally, which is hard to get from outside the project. So I opened #1356 to rebuild set values for each sample there, and I'm going to close this PR in favor of it.

Your tests and the analysis in #1348 helped a lot in finding the cases to cover. Thank you again, and sorry I couldn't merge this one.

If you'd still like to contribute, just let me know and I'll suggest another issue that would be a good fit.

@seongahjo seongahjo closed this Oct 30, 2026
@shin4141

Copy link
Copy Markdown
Author

Thanks for the explanation and for taking the time to review the work. I understand the reasoning behind the alternative approach.

I'm glad the investigation and tests were useful. I'll leave this issue here and won't take on another one for now.

Best of luck with #1356.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants