Provide unique IDs for all node info objects#1696
Open
Vampire wants to merge 2 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1696 +/- ##
============================================
+ Coverage 82.32% 82.34% +0.01%
- Complexity 4877 4879 +2
============================================
Files 474 474
Lines 15205 15212 +7
Branches 1935 1935
============================================
+ Hits 12518 12526 +8
Misses 1993 1993
+ Partials 694 693 -1
🚀 New features to boost your workflow:
|
Vampire
force-pushed
the
unique-node-ids
branch
3 times, most recently
from
595c6da to
3cacaf6
Compare
Member
|
What is the use-case? And shouldn't we have something that is stable across invocations, otherwise it's use is severely limited. |
Vampire
force-pushed
the
unique-node-ids
branch
from
3cacaf6 to
f207f8a
Compare
Member
Author
Well, everytime you need a unique ID during the test.
Well, the intended use by me was during one test run to relate things, without needing to build a unique ID yourself. Not sure whether we need stable unique ids for those. :-/ |
Vampire
force-pushed
the
unique-node-ids
branch
from
f207f8a to
9a9dc34
Compare
Member
Author
|
Also, that they are stable across invocations for spec, feature, and iteration info is mainly a side-effect. The goal was to have something descriptive that can the also easily be readable for example in the log output where I intend to use it. :-D |
Vampire
force-pushed
the
unique-node-ids
branch
from
9a9dc34 to
f91997e
Compare
Vampire
force-pushed
the
unique-node-ids
branch
2 times, most recently
from
82329d4 to
9e20670
Compare
Vampire
force-pushed
the
unique-node-ids
branch
2 times, most recently
from
0a54c3f to
53f0557
Compare
Vampire
force-pushed
the
unique-node-ids
branch
3 times, most recently
from
5c9ac8c to
02aff00
Compare
Vampire
force-pushed
the
unique-node-ids
branch
3 times, most recently
from
6e21a45 to
b3d7dbb
Compare
Vampire
force-pushed
the
unique-node-ids
branch
4 times, most recently
from
2caee62 to
6c400f0
Compare
Vampire
force-pushed
the
unique-node-ids
branch
4 times, most recently
from
0422e2b to
d6e3be7
Compare
Vampire
force-pushed
the
unique-node-ids
branch
2 times, most recently
from
937bf52 to
60bf169
Compare
Vampire
force-pushed
the
unique-node-ids
branch
2 times, most recently
from
3b8d246 to
b322fb9
Compare
Vampire
force-pushed
the
unique-node-ids
branch
2 times, most recently
from
ad0c185 to
92d0a32
Compare
Vampire
force-pushed
the
unique-node-ids
branch
4 times, most recently
from
3080aad to
06b7338
Compare
Vampire
force-pushed
the
unique-node-ids
branch
3 times, most recently
from
00a4141 to
307e4dc
Compare
Vampire
force-pushed
the
unique-node-ids
branch
3 times, most recently
from
a01553f to
f410524
Compare
Vampire
force-pushed
the
unique-node-ids
branch
from
f410524 to
f0ce472
Compare
Vampire
force-pushed
the
unique-node-ids
branch
from
f0ce472 to
d813cd1
Compare
Vampire
force-pushed
the
unique-node-ids
branch
2 times, most recently
from
968bbe9 to
c434efe
Compare
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@spock-core/src/main/java/org/spockframework/runtime/model/IterationInfo.java`:
- Around line 33-35: The iteration ID is being built with locale-sensitive
String.format in the IterationInfo constructor
(setUniqueId(String.format("%s[%d]", ...))), which can produce locale-specific
digits; change this to a locale-independent construction—either use simple
String concatenation (feature.getUniqueId() + "[" + iterationIndex + "]") or
call String.format(Locale.ROOT, "%s[%d]", feature.getUniqueId(),
iterationIndex)—so setUniqueId always receives a stable, locale-neutral
identifier.
In `@spock-core/src/main/java/org/spockframework/runtime/model/NodeInfo.java`:
- Around line 38-52: The public mutator setUniqueId on NodeInfo exposes and
allows replacing the node's uniqueId (and setting null/duplicates); remove or
restrict that mutator and enforce one-time, non-null assignment instead: either
make setUniqueId non-public (protected or package-private) or eliminate it and
assign uniqueId during construction, and if you keep an assignment method ensure
it throws on null or if uniqueId is already set (idempotent write-protection).
Update references to getUniqueId, setUniqueId and the uniqueId field accordingly
so only internal/runtime code can set the ID and callers cannot overwrite it.
- Around line 23-31: NodeInfo currently eagerly allocates a UUID in the field
declaration and exposes a public setUniqueId, causing wasted allocations
(IterationInfo immediately replaces it) and allowing external mutation; change
uniqueId to not be eagerly generated (e.g. initialize to null and lazily create
on first getUniqueId OR require subclasses to supply a semantic id via
constructor), replace the public setUniqueId with a non-public API (make it
private/protected or remove it) and, if desired, implement a cheap per-execution
id generator (static AtomicLong counter) that subclasses or getUniqueId can use
to produce lightweight unique values; update NodeInfo.getUniqueId/setters and
constructors of SpecInfo, FeatureInfo, IterationInfo accordingly to ensure
immutability of the identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d290a688-11d0-4184-abc2-413afcc836e3
📒 Files selected for processing (3)
spock-core/src/main/java/org/spockframework/runtime/SpecInfoBuilder.javaspock-core/src/main/java/org/spockframework/runtime/model/IterationInfo.javaspock-core/src/main/java/org/spockframework/runtime/model/NodeInfo.java
Comment on lines
+23
to
+31
| import java.util.UUID; | ||
|
|
||
| /** | ||
| * Base class for runtime information about an element in a Spock specification. | ||
| * | ||
| * @author Peter Niederwieser | ||
| */ | ||
| public abstract class NodeInfo<P extends NodeInfo, R extends AnnotatedElement> { | ||
| private String uniqueId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine NodeInfo.java
find . -type f -name "NodeInfo.java" | head -20Repository: spockframework/spock
Length of output: 137
🏁 Script executed:
# Check the full content of NodeInfo.java
if [ -f "spock-core/src/main/java/org/spockframework/runtime/model/NodeInfo.java" ]; then
cat -n "spock-core/src/main/java/org/spockframework/runtime/model/NodeInfo.java"
else
# Search for the file
find . -name "NodeInfo.java" -type f
fiRepository: spockframework/spock
Length of output: 3665
🏁 Script executed:
# Also check SpecInfoBuilder.java to see if it assigns semantic IDs
find . -type f -name "SpecInfoBuilder.java" | head -5Repository: spockframework/spock
Length of output: 138
🏁 Script executed:
# Check IterationInfo.java
find . -type f -name "IterationInfo.java" | head -5Repository: spockframework/spock
Length of output: 142
🏁 Script executed:
# Examine SpecInfoBuilder.java to find semantic ID assignments
cat -n "spock-core/src/main/java/org/spockframework/runtime/SpecInfoBuilder.java"Repository: spockframework/spock
Length of output: 10958
🏁 Script executed:
# Examine IterationInfo.java
cat -n "spock-core/src/main/java/org/spockframework/runtime/model/IterationInfo.java"Repository: spockframework/spock
Length of output: 5336
🏁 Script executed:
# Search for setUniqueId calls to verify overwriting of uniqueId
rg "setUniqueId" -n --context 3Repository: spockframework/spock
Length of output: 3267
Avoid eagerly generating a UUID in the base type.
SpecInfo, FeatureInfo, and IterationInfo each overwrite the default UUID with semantic IDs. On the per-iteration path, this causes unnecessary UUID allocation overhead—each IterationInfo constructor immediately discards the generated UUID. A per-execution counter would satisfy the documented uniqueness requirement without the allocation cost.
Additionally, the public setUniqueId() setter (lines 50–52) weakens the API contract by allowing arbitrary mutation of what should be an immutable identifier.
💡 One possible simplification
-import java.util.UUID;
+import java.util.concurrent.atomic.AtomicLong;
public abstract class NodeInfo<P extends NodeInfo, R extends AnnotatedElement> {
- private String uniqueId = UUID.randomUUID().toString();
+ private static final AtomicLong NEXT_ID = new AtomicLong();
+ private String uniqueId = "node-" + NEXT_ID.incrementAndGet();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spock-core/src/main/java/org/spockframework/runtime/model/NodeInfo.java`
around lines 23 - 31, NodeInfo currently eagerly allocates a UUID in the field
declaration and exposes a public setUniqueId, causing wasted allocations
(IterationInfo immediately replaces it) and allowing external mutation; change
uniqueId to not be eagerly generated (e.g. initialize to null and lazily create
on first getUniqueId OR require subclasses to supply a semantic id via
constructor), replace the public setUniqueId with a non-public API (make it
private/protected or remove it) and, if desired, implement a cheap per-execution
id generator (static AtomicLong counter) that subclasses or getUniqueId can use
to produce lightweight unique values; update NodeInfo.getUniqueId/setters and
constructors of SpecInfo, FeatureInfo, IterationInfo accordingly to ensure
immutability of the identifier.
This comment has been minimized.
This comment has been minimized.
🚨 TestLens detected 4 failed tests 🚨Here is what you can do:
Test SummaryVerify Branches and PRs / Build and Verify (2.5, 8, windows-latest) > :spock-specs:test
Verify Branches and PRs / Build and Verify (3.0, 8, windows-latest) > :spock-specs:test
Verify Branches and PRs / Build and Verify (4.0, 8, windows-latest) > :spock-specs:test
🏷️ Commit: da3f5f4 Test FailuresAsyncConditionsSpec > passing example - check passes (:spock-specs:test in Verify Branches and PRs / Build and Verify (2.5, 8, windows-latest))AsyncConditionsSpec > multiple passing evals (:spock-specs:test in Verify Branches and PRs / Build and Verify (3.0, 8, windows-latest))BlockingVariablesSpec > failing example 1 - variable has wrong value (:spock-specs:test in Verify Branches and PRs / Build and Verify (3.0, 8, windows-latest))BlockingVariablesSpec > passing example 2 - variable is read before it is written (:spock-specs:test in Verify Branches and PRs / Build and Verify (4.0, 8, windows-latest))Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit