Provide unique IDs for all node info objects#1696
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1696 +/- ##
============================================
+ Coverage 82.16% 82.24% +0.08%
- Complexity 4822 4829 +7
============================================
Files 472 472
Lines 15036 15043 +7
Branches 1905 1905
============================================
+ Hits 12354 12372 +18
+ Misses 1989 1983 -6
+ Partials 693 688 -5
🚀 New features to boost your workflow:
|
Vampire
force-pushed
the
unique-node-ids
branch
3 times, most recently
from
595c6da to
3cacaf6
Compare
|
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
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
|
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAssigns hierarchical unique IDs: NodeInfo gains a UUID-default Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as SpecInfoBuilder
participant Spec as SpecInfo
participant Feature as FeatureInfo
participant Iteration as IterationInfo
Builder->>Spec: buildSpec(clazz)
Spec->>Spec: setUniqueId(className)
Builder->>Feature: createFeature(method)
Feature->>Feature: setUniqueId(specUniqueId + "." + methodName)
Feature->>Iteration: createIteration(index)
Iteration->>Iteration: setUniqueId(featureUniqueId + "[" + index + "]")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
| 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.
🔎 No tests executed 🔎🏷️ Commit: 0f36696 Learn more about TestLens at testlens.app. |
2 participants
Summary by CodeRabbit