[Tests] Add unit tests for PlayerStatisticData, ReloadableParser, ItemPluginType, ChainPlayerContextFilter#33
Open
DiamondDagger590 wants to merge 1 commit into
Open
Conversation
…mPluginType, ChainPlayerContextFilter https://claude.ai/code/session_012thebA7utDjU8bJZqBUAB9
WalkthroughThis PR adds four new JUnit 5 test classes covering enum validation, YAML configuration parsing, statistic data mutations with event handling, and filter chain composition. The tests verify correct behavior across edge cases including case-insensitive input handling, event cancellation semantics, and sequential filter application. ChangesTest Coverage Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Testing ReviewI'll systematically apply every checklist item to the four test files and the production classes they exercise. Checklist ApplicationCoverage CompletenessCONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: There is no test for CONCERN: There is no test verifying CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: No test file is present for the MockBukkit UsageCONCERN: None of the four test files call Naming ConventionCONCERN: Several test method names in CONCERN: Several test method names in SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewNo security concerns found. The diff contains only test code (
The test helpers ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/diamonddagger590/mccore/builder/item/ItemPluginTypeTest.java`:
- Around line 78-83: Test currently omits the null input case for
ItemPluginType.fromName; update the parameterized test method
fromName_returnsNone_forUnrecognizedName (or add a new dedicated test) to
include a null case by adding `@NullSource` to the `@ValueSource` annotation (or
creating a small test that calls ItemPluginType.fromName(null) and asserts
ItemPluginType.NONE), ensuring the null-path behavior is explicitly verified.
In
`@src/test/java/com/diamonddagger590/mccore/configuration/common/ReloadableParserTest.java`:
- Around line 35-81: Add unit tests covering edge cases for ReloadableParser:
create new test methods that construct YamlDocument instances with (1) missing
or null EQUATION_ROUTE, (2) empty or invalid expressions (e.g., empty string,
non-parsable text), and (3) boundary numeric values (zero, negative numbers,
very large/small doubles) and assert expected behavior (exceptions thrown or
default/constant parser values). Use ReloadableParser(yaml, EQUATION_ROUTE),
call getContent(), parser.getValue(), parser.setVariable(...) and
reloadContent() where appropriate, and
assertNull/assertThrows/assertEquals/assertNotSame to verify correct handling of
null/missing routes, invalid expressions, and numeric boundaries.
- Line 37: Rename the test methods that currently use custom names to follow the
givenContext_whenAction_thenOutcome pattern: change
constructor_createsParser_thatEvaluatesEquation() and the three other test
methods referenced in this file (lines noted in the review) to descriptive names
that start with "given", describe the context, then "_when_" for the action, and
"_then_" for the expected outcome; update the method identifiers (method
declarations) accordingly in ReloadableParserTest so the `@Test` annotations
remain on the renamed methods and any internal references to those method names
are adjusted.
In
`@src/test/java/com/diamonddagger590/mccore/statistic/PlayerStatisticDataAdditionalTest.java`:
- Around line 81-412: The test method names in PlayerStatisticDataAdditionalTest
(e.g., getUUID_returnsConstructorUUID,
getValue_returnsDefault_whenNoStoredValue,
bulkIncrementLong_delegatesToIncrementLong,
addToSet_worksCorrectly_whenPopulatedWithPlainHashSet,
incrementInt_doesNotChange_whenCancelled,
setMaxLong_doesNotChange_whenCancelled,
getModifiedEntries_skipsKey_whenValueIsNull,
incrementLong_throws_forUnregisteredKey, addToSet_throws_forUnregisteredKey,
etc.) do not follow the required givenContext_whenAction_thenOutcome naming
convention; rename each test method to the form
given<Context>_when<Action>_then<Outcome> (for example
getUUID_returnsConstructorUUID ->
givenPlayerStatisticData_whenGetUUID_thenReturnsConstructorUUID,
getValue_returnsDefault_whenNoStoredValue ->
givenNoStoredValueAndRegisteredStat_whenGetValue_thenReturnsDefault,
bulkIncrementLong_delegatesToIncrementLong ->
givenLongStat_whenBulkIncrementLong_thenDelegatesAndAccumulates, and similarly
for the others) so every test method in this class follows the required pattern.
- Around line 89-412: Add unit tests to PlayerStatisticDataAdditionalTest
covering null and boundary numeric inputs: (1) verify null NamespacedKey or null
value passed to incrementInt/incrementLong/incrementDouble,
setMaxInt/setMaxLong/setMaxDouble, setTimestampIfAbsent, addToSet, and
removeFromSet produce the expected exception (e.g., NullPointerException or
IllegalArgumentException) by calling incrementInt/incrementLong/incrementDouble,
setMaxInt/setMaxLong/setMaxDouble, setTimestampIfAbsent, addToSet, removeFromSet
with null key or null element; (2) add tests for zero, negative, and max values
for incrementInt/incrementLong/incrementDouble and setMax* to assert correct
result or thrown exception (use
Integer.MAX_VALUE/Long.MAX_VALUE/Double.MAX_VALUE where appropriate); and (3)
add tests for empty and null collections when populating via populateFromEntries
and when adding/removing from set to ensure addToSet/removeFromSet behave
correctly. Use the existing helper keys (INT_KEY, LONG_KEY, DOUBLE_KEY,
TIMESTAMP_KEY, SET_KEY) and methods like incrementInt, incrementLong,
incrementDouble, setMaxInt, setMaxLong, setMaxDouble, setTimestampIfAbsent,
addToSet, removeFromSet, and populateFromEntries to locate targets.
- Around line 337-347: The test's assertions contradict its description: it
intends to verify that getModifiedEntries() skips a key whose value was
cleared/null, but it currently sets INT_KEY back to 99 and asserts presence;
update the test to actually clear the key (e.g., call the method that clears the
value or setValue(INT_KEY, null) after populateFromEntries(Map.of())) and then
call getModifiedEntries() and assert that the resulting Map does NOT contain
INT_KEY; reference the existing helpers and symbols data.setValue,
data.populateFromEntries, data.getModifiedEntries, INT_KEY, and StatisticEntry
when making this change.
In
`@src/test/java/com/diamonddagger590/mccore/util/filter/ChainPlayerContextFilterTest.java`:
- Around line 138-140: The TestCorePlayer(UUID uuid) constructor parameter
should be annotated with `@NotNull` (IntelliJ annotations v12); update the
constructor signature in class TestCorePlayer to import and apply
org.jetbrains.annotations.NotNull to the uuid parameter so the non-null contract
is explicit and matches project guidelines.
- Around line 35-134: Add a test method (e.g., filter_throwsWhenInputIsNull) in
ChainPlayerContextFilterTest that constructs a ChainPlayerContextFilter (use a
noop PlayerContextFilter or existing noopFilter) and asserts that calling
chain.filter(testPlayer, null) throws the expected exception (use
assertThrows(NullPointerException.class, ...) to verify the null-input
contract). Reference ChainPlayerContextFilter.filter and the test fixture
testPlayer when adding the assertion.
- Around line 37-124: The test method names do not follow the required
givenContext_whenAction_thenOutcome convention; rename each test method to that
pattern (e.g., filter_appliesSingleFilter_correctly ->
givenSingleFilter_whenFiltering_thenAppliesCorrectly,
filter_appliesMultipleFilters_inOrder ->
givenTwoFilters_whenFiltering_thenAppliesInOrder,
filter_orderMatters_forFilterChain ->
givenFiltersInReverseOrder_whenFiltering_thenOrderMatters,
filter_returnsEmpty_whenInputIsEmpty ->
givenEmptyInput_whenFiltering_thenReturnsEmpty,
filter_returnsEmpty_whenFirstFilterRemovesAll ->
givenFirstFilterRemovesAll_whenChaining_thenReturnsEmpty,
filter_appliesThreeFilters_sequentially ->
givenThreeFilters_whenFiltering_thenAllAppliedSequentially,
filter_passesPlayerContext_toEachFilter ->
givenStringFilters_whenFilteringWithPlayerContext_thenPlayerAccessible); update
the method names in ChainPlayerContextFilterTest accordingly and run tests to
ensure no references (e.g., `@DisplayName` remains) are broken.
- Around line 64-77: The test "filter_orderMatters_forFilterChain" currently
uses two commutative predicates (greaterThanThree and evenOnly) so it doesn't
prove order sensitivity; replace the filters with order-dependent ones (for
example a take-first-N filter and an even-only filter) so chaining order changes
the result: implement something like takeFirstThree = (player,list) ->
list.stream().limit(3).collect(...) and keep evenOnly = (player,list) ->
list.stream().filter(i -> i % 2 == 0).collect(...), create the
ChainPlayerContextFilter with (takeFirstThree, evenOnly), call
chain.filter(testPlayer, List.of(...)) and assert the expected different result
(e.g. when using List.of(1,2,3,4,5,6) expect [2] for takeFirstThree then
evenOnly but a different list if reversed), updating the assertion accordingly;
reference ChainPlayerContextFilter, filter_orderMatters_forFilterChain,
takeFirstThree, evenOnly, and the filter(...) method to locate and change the
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9d2a4aa-4426-4e34-9d50-a8e42b9eaa61
📒 Files selected for processing (4)
src/test/java/com/diamonddagger590/mccore/builder/item/ItemPluginTypeTest.javasrc/test/java/com/diamonddagger590/mccore/configuration/common/ReloadableParserTest.javasrc/test/java/com/diamonddagger590/mccore/statistic/PlayerStatisticDataAdditionalTest.javasrc/test/java/com/diamonddagger590/mccore/util/filter/ChainPlayerContextFilterTest.java
Comment on lines
+78
to
+83
| @ParameterizedTest | ||
| @ValueSource(strings = {"unknown", "oraxen", "mmoitems", "customitems", "", " "}) | ||
| @DisplayName("Given an unrecognized name, when calling fromName, then returns NONE as fallback") | ||
| void fromName_returnsNone_forUnrecognizedName(String name) { | ||
| assertEquals(ItemPluginType.NONE, ItemPluginType.fromName(name)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add null-input coverage for fromName edge-case contract.
Line 78-83 currently covers unknown/blank values but not null. Please extend this parameterized test with @NullSource (or add a dedicated null test) so the null-path behavior is explicitly verified.
As per coding guidelines, "Cover edge cases in tests: null inputs, empty collections, zero/negative numeric inputs, and max/limit values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/builder/item/ItemPluginTypeTest.java`
around lines 78 - 83, Test currently omits the null input case for
ItemPluginType.fromName; update the parameterized test method
fromName_returnsNone_forUnrecognizedName (or add a new dedicated test) to
include a null case by adding `@NullSource` to the `@ValueSource` annotation (or
creating a small test that calls ItemPluginType.fromName(null) and asserts
ItemPluginType.NONE), ensuring the null-path behavior is explicitly verified.
Source: Coding guidelines
Comment on lines
+35
to
+81
| @Test | ||
| @DisplayName("Given a YAML with a simple equation, when constructing ReloadableParser, then parser evaluates correctly") | ||
| void constructor_createsParser_thatEvaluatesEquation() throws IOException { | ||
| YamlDocument yaml = createYamlDocument("equation: '2 + 3'"); | ||
| ReloadableParser reloadableParser = new ReloadableParser(yaml, EQUATION_ROUTE); | ||
|
|
||
| Parser parser = reloadableParser.getContent(); | ||
| assertNotNull(parser); | ||
| assertEquals(5.0, parser.getValue(), 0.001); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a YAML with a variable equation, when constructing ReloadableParser, then parser accepts variables") | ||
| void constructor_createsParser_thatAcceptsVariables() throws IOException { | ||
| YamlDocument yaml = createYamlDocument("equation: 'x * 2'"); | ||
| ReloadableParser reloadableParser = new ReloadableParser(yaml, EQUATION_ROUTE); | ||
|
|
||
| Parser parser = reloadableParser.getContent(); | ||
| assertNotNull(parser); | ||
| parser.setVariable("x", 5.0); | ||
| assertEquals(10.0, parser.getValue(), 0.001); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an updated YAML, when reloading, then parser reflects new equation") | ||
| void reloadContent_updatesParser_whenYamlChanges() throws IOException { | ||
| YamlDocument yaml = createYamlDocument("equation: '1 + 1'"); | ||
| ReloadableParser reloadableParser = new ReloadableParser(yaml, EQUATION_ROUTE); | ||
|
|
||
| assertEquals(2.0, reloadableParser.getContent().getValue(), 0.001); | ||
|
|
||
| yaml.set(EQUATION_ROUTE, "10 * 3"); | ||
| Parser oldParser = reloadableParser.getContent(); | ||
| reloadableParser.reloadContent(); | ||
|
|
||
| assertNotSame(oldParser, reloadableParser.getContent()); | ||
| assertEquals(30.0, reloadableParser.getContent().getValue(), 0.001); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a YAML with a constant equation, when constructing ReloadableParser, then returns constant value") | ||
| void constructor_handlesConstantEquation() throws IOException { | ||
| YamlDocument yaml = createYamlDocument("equation: '42'"); | ||
| ReloadableParser reloadableParser = new ReloadableParser(yaml, EQUATION_ROUTE); | ||
|
|
||
| assertEquals(42.0, reloadableParser.getContent().getValue(), 0.001); | ||
| } |
There was a problem hiding this comment.
Add edge-case tests for parser inputs and route/value states.
The suite currently validates only happy paths; add edge cases (e.g., missing/null equation route, invalid/empty expression, and boundary numeric values) to satisfy test coverage requirements.
As per coding guidelines, "Cover edge cases in tests: null inputs, empty collections, zero/negative numeric inputs, and max/limit values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/configuration/common/ReloadableParserTest.java`
around lines 35 - 81, Add unit tests covering edge cases for ReloadableParser:
create new test methods that construct YamlDocument instances with (1) missing
or null EQUATION_ROUTE, (2) empty or invalid expressions (e.g., empty string,
non-parsable text), and (3) boundary numeric values (zero, negative numbers,
very large/small doubles) and assert expected behavior (exceptions thrown or
default/constant parser values). Use ReloadableParser(yaml, EQUATION_ROUTE),
call getContent(), parser.getValue(), parser.setVariable(...) and
reloadContent() where appropriate, and
assertNull/assertThrows/assertEquals/assertNotSame to verify correct handling of
null/missing routes, invalid expressions, and numeric boundaries.
Source: Coding guidelines
|
|
||
| @Test | ||
| @DisplayName("Given a YAML with a simple equation, when constructing ReloadableParser, then parser evaluates correctly") | ||
| void constructor_createsParser_thatEvaluatesEquation() throws IOException { |
There was a problem hiding this comment.
Rename test methods to the required givenContext_whenAction_thenOutcome pattern.
Line 37, Line 48, Line 60, and Line 76 currently use custom naming that does not follow the project’s required test method convention.
As per coding guidelines, "Follow the givenContext_whenAction_thenOutcome naming convention for test methods."
Also applies to: 48-48, 60-60, 76-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/configuration/common/ReloadableParserTest.java`
at line 37, Rename the test methods that currently use custom names to follow
the givenContext_whenAction_thenOutcome pattern: change
constructor_createsParser_thatEvaluatesEquation() and the three other test
methods referenced in this file (lines noted in the review) to descriptive names
that start with "given", describe the context, then "_when_" for the action, and
"_then_" for the expected outcome; update the method identifiers (method
declarations) accordingly in ReloadableParserTest so the `@Test` annotations
remain on the renamed methods and any internal references to those method names
are adjusted.
Source: Coding guidelines
Comment on lines
+81
to
+412
| @Test | ||
| @DisplayName("Given a PlayerStatisticData, when calling getUUID, then returns the UUID passed to constructor") | ||
| void getUUID_returnsConstructorUUID() { | ||
| assertEquals(PLAYER_UUID, data.getUUID()); | ||
| } | ||
|
|
||
| // --- getValue (raw Object accessor) --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given no stored value and a registered stat, when calling getValue, then returns the default value") | ||
| void getValue_returnsDefault_whenNoStoredValue() { | ||
| assertTrue(data.getValue(INT_KEY).isPresent()); | ||
| assertEquals(0, data.getValue(INT_KEY).get()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a stored value, when calling getValue, then returns that stored value") | ||
| void getValue_returnsStoredValue_afterSet() { | ||
| data.setValue(INT_KEY, 42); | ||
| assertEquals(42, data.getValue(INT_KEY).get()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when calling getValue, then returns empty") | ||
| void getValue_returnsEmpty_forUnregisteredKey() { | ||
| assertTrue(data.getValue(key("test", "nonexistent")).isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a string stat with stored value, when calling getValue, then returns the string") | ||
| void getValue_returnsString_whenStringStatStored() { | ||
| data.setValue(STRING_KEY, "hello"); | ||
| assertEquals("hello", data.getValue(STRING_KEY).get()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a timestamp stat with stored value, when calling getValue, then returns the Instant") | ||
| void getValue_returnsInstant_whenTimestampStatStored() { | ||
| Instant now = Instant.now(); | ||
| data.setValue(TIMESTAMP_KEY, now); | ||
| assertEquals(now, data.getValue(TIMESTAMP_KEY).get()); | ||
| } | ||
|
|
||
| // --- bulkIncrementLong --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a long stat, when calling bulkIncrementLong, then it delegates to incrementLong") | ||
| void bulkIncrementLong_delegatesToIncrementLong() { | ||
| data.bulkIncrementLong(LONG_KEY, 50L); | ||
| assertEquals(50L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a long stat, when calling bulkIncrementLong multiple times, then values accumulate") | ||
| void bulkIncrementLong_accumulates_whenCalledMultipleTimes() { | ||
| data.bulkIncrementLong(LONG_KEY, 10L); | ||
| data.bulkIncrementLong(LONG_KEY, 20L); | ||
| assertEquals(30L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a long stat, when calling bulkIncrementLong, then fires pre and post events with INCREMENT type") | ||
| void bulkIncrementLong_firesEvents_withIncrementModificationType() { | ||
| data.bulkIncrementLong(LONG_KEY, 5L); | ||
| assertEquals(2, firedEvents.size()); | ||
| assertTrue(firedEvents.get(0) instanceof StatisticModifyEvent); | ||
| StatisticModifyEvent preEvent = (StatisticModifyEvent) firedEvents.get(0); | ||
| assertEquals(ModificationType.INCREMENT, preEvent.getModificationType()); | ||
| assertTrue(firedEvents.get(1) instanceof PostStatisticModifyEvent); | ||
| } | ||
|
|
||
| // --- getOrCreateSet with plain HashSet (non-LinkedHashSet) --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a set stat populated with a plain HashSet via entries, when adding to the set, then works correctly") | ||
| void addToSet_worksCorrectly_whenPopulatedWithPlainHashSet() { | ||
| Set<String> plainSet = new HashSet<>(); | ||
| plainSet.add("existing"); | ||
| data.populateFromEntries(Map.of(SET_KEY, new StatisticEntry(SET_KEY, StatisticType.SET_STRING, plainSet))); | ||
| assertTrue(data.addToSet(SET_KEY, "new_element")); | ||
| Set<String> result = data.getSetValue(SET_KEY).get(); | ||
| assertTrue(result.contains("existing")); | ||
| assertTrue(result.contains("new_element")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a set stat populated with a plain HashSet, when removing from the set, then works correctly") | ||
| void removeFromSet_worksCorrectly_whenPopulatedWithPlainHashSet() { | ||
| Set<String> plainSet = new HashSet<>(); | ||
| plainSet.add("target"); | ||
| data.populateFromEntries(Map.of(SET_KEY, new StatisticEntry(SET_KEY, StatisticType.SET_STRING, plainSet))); | ||
| assertTrue(data.removeFromSet(SET_KEY, "target")); | ||
| assertFalse(data.getSetValue(SET_KEY).get().contains("target")); | ||
| } | ||
|
|
||
| // --- Increment cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling event dispatcher, when incrementing int, then value does not change") | ||
| void incrementInt_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.incrementInt(INT_KEY, 10); | ||
| assertEquals(0, data.getIntValue(INT_KEY).orElse(0)); | ||
| assertFalse(data.isDirty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling event dispatcher, when incrementing double, then value does not change") | ||
| void incrementDouble_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.incrementDouble(DOUBLE_KEY, 5.0); | ||
| assertEquals(0.0, data.getDoubleValue(DOUBLE_KEY).orElse(0.0), 0.001); | ||
| assertFalse(data.isDirty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling event dispatcher, when incrementing long, then value does not change") | ||
| void incrementLong_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.incrementLong(LONG_KEY, 100L); | ||
| assertEquals(0L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| assertFalse(data.isDirty()); | ||
| } | ||
|
|
||
| // --- setMax cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting max long, then value does not change") | ||
| void setMaxLong_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setMaxLong(LONG_KEY, 100L); | ||
| assertEquals(0L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting max int, then value does not change") | ||
| void setMaxInt_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setMaxInt(INT_KEY, 100); | ||
| assertEquals(0, data.getIntValue(INT_KEY).orElse(0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting max double, then value does not change") | ||
| void setMaxDouble_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setMaxDouble(DOUBLE_KEY, 100.0); | ||
| assertEquals(0.0, data.getDoubleValue(DOUBLE_KEY).orElse(0.0), 0.001); | ||
| } | ||
|
|
||
| // --- setTimestampIfAbsent cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting timestamp if absent, then value does not change") | ||
| void setTimestampIfAbsent_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setTimestampIfAbsent(TIMESTAMP_KEY, Instant.now()); | ||
| assertEquals(Instant.EPOCH, data.getTimestampValue(TIMESTAMP_KEY).orElse(Instant.EPOCH)); | ||
| } | ||
|
|
||
| // --- addToSet cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when adding to set, then returns false and set is unchanged") | ||
| void addToSet_returnsFalse_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| assertFalse(data.addToSet(SET_KEY, "value")); | ||
| assertTrue(data.getSetValue(SET_KEY).get().isEmpty()); | ||
| } | ||
|
|
||
| // --- removeFromSet cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a set with an element and a cancelling dispatcher, when removing, then element remains") | ||
| void removeFromSet_doesNotRemove_whenCancelled() { | ||
| data.addToSet(SET_KEY, "keep_me"); | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.populateFromEntries(Map.of(SET_KEY, new StatisticEntry(SET_KEY, StatisticType.SET_STRING, new LinkedHashSet<>(Set.of("keep_me"))))); | ||
| assertFalse(data.removeFromSet(SET_KEY, "keep_me")); | ||
| assertTrue(data.getSetValue(SET_KEY).get().contains("keep_me")); | ||
| } | ||
|
|
||
| // --- getModifiedEntries with null value --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a dirty key whose value was cleared, when getting modified entries, then that key is skipped") | ||
| void getModifiedEntries_skipsKey_whenValueIsNull() { | ||
| data.setValue(INT_KEY, 42); | ||
| assertTrue(data.isDirty()); | ||
| data.populateFromEntries(Map.of()); | ||
| data.setValue(INT_KEY, 99); | ||
| Map<NamespacedKey, StatisticEntry> modified = data.getModifiedEntries(); | ||
| assertTrue(modified.containsKey(INT_KEY)); | ||
| assertEquals(99, modified.get(INT_KEY).getAsInt()); | ||
| } | ||
|
|
||
| // --- Increment throws for unregistered key --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when incrementing long, then throws StatisticNotRegisteredException") | ||
| void incrementLong_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.incrementLong(key("test", "nonexistent"), 1L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when incrementing int, then throws StatisticNotRegisteredException") | ||
| void incrementInt_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.incrementInt(key("test", "nonexistent"), 1)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when incrementing double, then throws StatisticNotRegisteredException") | ||
| void incrementDouble_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.incrementDouble(key("test", "nonexistent"), 1.0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting max long, then throws StatisticNotRegisteredException") | ||
| void setMaxLong_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setMaxLong(key("test", "nonexistent"), 1L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting max int, then throws StatisticNotRegisteredException") | ||
| void setMaxInt_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setMaxInt(key("test", "nonexistent"), 1)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting max double, then throws StatisticNotRegisteredException") | ||
| void setMaxDouble_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setMaxDouble(key("test", "nonexistent"), 1.0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting timestamp if absent, then throws StatisticNotRegisteredException") | ||
| void setTimestampIfAbsent_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setTimestampIfAbsent(key("test", "nonexistent"), Instant.now())); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when adding to set, then throws StatisticNotRegisteredException") | ||
| void addToSet_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.addToSet(key("test", "nonexistent"), "value")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when removing from set, then throws StatisticNotRegisteredException") | ||
| void removeFromSet_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.removeFromSet(key("test", "nonexistent"), "value")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Rename test methods to the required givenContext_whenAction_thenOutcome format.
Current names (for example, getUUID_returnsConstructorUUID) do not follow the required repository naming convention for test methods.
As per coding guidelines, "Follow the givenContext_whenAction_thenOutcome naming convention for test methods (e.g., givenEmptyPage_whenGetSlots_thenReturnsEmpty)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/statistic/PlayerStatisticDataAdditionalTest.java`
around lines 81 - 412, The test method names in
PlayerStatisticDataAdditionalTest (e.g., getUUID_returnsConstructorUUID,
getValue_returnsDefault_whenNoStoredValue,
bulkIncrementLong_delegatesToIncrementLong,
addToSet_worksCorrectly_whenPopulatedWithPlainHashSet,
incrementInt_doesNotChange_whenCancelled,
setMaxLong_doesNotChange_whenCancelled,
getModifiedEntries_skipsKey_whenValueIsNull,
incrementLong_throws_forUnregisteredKey, addToSet_throws_forUnregisteredKey,
etc.) do not follow the required givenContext_whenAction_thenOutcome naming
convention; rename each test method to the form
given<Context>_when<Action>_then<Outcome> (for example
getUUID_returnsConstructorUUID ->
givenPlayerStatisticData_whenGetUUID_thenReturnsConstructorUUID,
getValue_returnsDefault_whenNoStoredValue ->
givenNoStoredValueAndRegisteredStat_whenGetValue_thenReturnsDefault,
bulkIncrementLong_delegatesToIncrementLong ->
givenLongStat_whenBulkIncrementLong_thenDelegatesAndAccumulates, and similarly
for the others) so every test method in this class follows the required pattern.
Source: Coding guidelines
Comment on lines
+89
to
+412
| @Test | ||
| @DisplayName("Given no stored value and a registered stat, when calling getValue, then returns the default value") | ||
| void getValue_returnsDefault_whenNoStoredValue() { | ||
| assertTrue(data.getValue(INT_KEY).isPresent()); | ||
| assertEquals(0, data.getValue(INT_KEY).get()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a stored value, when calling getValue, then returns that stored value") | ||
| void getValue_returnsStoredValue_afterSet() { | ||
| data.setValue(INT_KEY, 42); | ||
| assertEquals(42, data.getValue(INT_KEY).get()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when calling getValue, then returns empty") | ||
| void getValue_returnsEmpty_forUnregisteredKey() { | ||
| assertTrue(data.getValue(key("test", "nonexistent")).isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a string stat with stored value, when calling getValue, then returns the string") | ||
| void getValue_returnsString_whenStringStatStored() { | ||
| data.setValue(STRING_KEY, "hello"); | ||
| assertEquals("hello", data.getValue(STRING_KEY).get()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a timestamp stat with stored value, when calling getValue, then returns the Instant") | ||
| void getValue_returnsInstant_whenTimestampStatStored() { | ||
| Instant now = Instant.now(); | ||
| data.setValue(TIMESTAMP_KEY, now); | ||
| assertEquals(now, data.getValue(TIMESTAMP_KEY).get()); | ||
| } | ||
|
|
||
| // --- bulkIncrementLong --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a long stat, when calling bulkIncrementLong, then it delegates to incrementLong") | ||
| void bulkIncrementLong_delegatesToIncrementLong() { | ||
| data.bulkIncrementLong(LONG_KEY, 50L); | ||
| assertEquals(50L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a long stat, when calling bulkIncrementLong multiple times, then values accumulate") | ||
| void bulkIncrementLong_accumulates_whenCalledMultipleTimes() { | ||
| data.bulkIncrementLong(LONG_KEY, 10L); | ||
| data.bulkIncrementLong(LONG_KEY, 20L); | ||
| assertEquals(30L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a long stat, when calling bulkIncrementLong, then fires pre and post events with INCREMENT type") | ||
| void bulkIncrementLong_firesEvents_withIncrementModificationType() { | ||
| data.bulkIncrementLong(LONG_KEY, 5L); | ||
| assertEquals(2, firedEvents.size()); | ||
| assertTrue(firedEvents.get(0) instanceof StatisticModifyEvent); | ||
| StatisticModifyEvent preEvent = (StatisticModifyEvent) firedEvents.get(0); | ||
| assertEquals(ModificationType.INCREMENT, preEvent.getModificationType()); | ||
| assertTrue(firedEvents.get(1) instanceof PostStatisticModifyEvent); | ||
| } | ||
|
|
||
| // --- getOrCreateSet with plain HashSet (non-LinkedHashSet) --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a set stat populated with a plain HashSet via entries, when adding to the set, then works correctly") | ||
| void addToSet_worksCorrectly_whenPopulatedWithPlainHashSet() { | ||
| Set<String> plainSet = new HashSet<>(); | ||
| plainSet.add("existing"); | ||
| data.populateFromEntries(Map.of(SET_KEY, new StatisticEntry(SET_KEY, StatisticType.SET_STRING, plainSet))); | ||
| assertTrue(data.addToSet(SET_KEY, "new_element")); | ||
| Set<String> result = data.getSetValue(SET_KEY).get(); | ||
| assertTrue(result.contains("existing")); | ||
| assertTrue(result.contains("new_element")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a set stat populated with a plain HashSet, when removing from the set, then works correctly") | ||
| void removeFromSet_worksCorrectly_whenPopulatedWithPlainHashSet() { | ||
| Set<String> plainSet = new HashSet<>(); | ||
| plainSet.add("target"); | ||
| data.populateFromEntries(Map.of(SET_KEY, new StatisticEntry(SET_KEY, StatisticType.SET_STRING, plainSet))); | ||
| assertTrue(data.removeFromSet(SET_KEY, "target")); | ||
| assertFalse(data.getSetValue(SET_KEY).get().contains("target")); | ||
| } | ||
|
|
||
| // --- Increment cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling event dispatcher, when incrementing int, then value does not change") | ||
| void incrementInt_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.incrementInt(INT_KEY, 10); | ||
| assertEquals(0, data.getIntValue(INT_KEY).orElse(0)); | ||
| assertFalse(data.isDirty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling event dispatcher, when incrementing double, then value does not change") | ||
| void incrementDouble_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.incrementDouble(DOUBLE_KEY, 5.0); | ||
| assertEquals(0.0, data.getDoubleValue(DOUBLE_KEY).orElse(0.0), 0.001); | ||
| assertFalse(data.isDirty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling event dispatcher, when incrementing long, then value does not change") | ||
| void incrementLong_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.incrementLong(LONG_KEY, 100L); | ||
| assertEquals(0L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| assertFalse(data.isDirty()); | ||
| } | ||
|
|
||
| // --- setMax cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting max long, then value does not change") | ||
| void setMaxLong_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setMaxLong(LONG_KEY, 100L); | ||
| assertEquals(0L, data.getLongValue(LONG_KEY).orElse(0L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting max int, then value does not change") | ||
| void setMaxInt_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setMaxInt(INT_KEY, 100); | ||
| assertEquals(0, data.getIntValue(INT_KEY).orElse(0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting max double, then value does not change") | ||
| void setMaxDouble_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setMaxDouble(DOUBLE_KEY, 100.0); | ||
| assertEquals(0.0, data.getDoubleValue(DOUBLE_KEY).orElse(0.0), 0.001); | ||
| } | ||
|
|
||
| // --- setTimestampIfAbsent cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when setting timestamp if absent, then value does not change") | ||
| void setTimestampIfAbsent_doesNotChange_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.setTimestampIfAbsent(TIMESTAMP_KEY, Instant.now()); | ||
| assertEquals(Instant.EPOCH, data.getTimestampValue(TIMESTAMP_KEY).orElse(Instant.EPOCH)); | ||
| } | ||
|
|
||
| // --- addToSet cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a cancelling dispatcher, when adding to set, then returns false and set is unchanged") | ||
| void addToSet_returnsFalse_whenCancelled() { | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| assertFalse(data.addToSet(SET_KEY, "value")); | ||
| assertTrue(data.getSetValue(SET_KEY).get().isEmpty()); | ||
| } | ||
|
|
||
| // --- removeFromSet cancellation --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a set with an element and a cancelling dispatcher, when removing, then element remains") | ||
| void removeFromSet_doesNotRemove_whenCancelled() { | ||
| data.addToSet(SET_KEY, "keep_me"); | ||
| data = new PlayerStatisticData( | ||
| PLAYER_UUID, | ||
| event -> { | ||
| if (event instanceof StatisticModifyEvent sme) { | ||
| sme.setCancelled(true); | ||
| } | ||
| }, | ||
| id -> mockPlayer | ||
| ); | ||
| data.populateFromEntries(Map.of(SET_KEY, new StatisticEntry(SET_KEY, StatisticType.SET_STRING, new LinkedHashSet<>(Set.of("keep_me"))))); | ||
| assertFalse(data.removeFromSet(SET_KEY, "keep_me")); | ||
| assertTrue(data.getSetValue(SET_KEY).get().contains("keep_me")); | ||
| } | ||
|
|
||
| // --- getModifiedEntries with null value --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given a dirty key whose value was cleared, when getting modified entries, then that key is skipped") | ||
| void getModifiedEntries_skipsKey_whenValueIsNull() { | ||
| data.setValue(INT_KEY, 42); | ||
| assertTrue(data.isDirty()); | ||
| data.populateFromEntries(Map.of()); | ||
| data.setValue(INT_KEY, 99); | ||
| Map<NamespacedKey, StatisticEntry> modified = data.getModifiedEntries(); | ||
| assertTrue(modified.containsKey(INT_KEY)); | ||
| assertEquals(99, modified.get(INT_KEY).getAsInt()); | ||
| } | ||
|
|
||
| // --- Increment throws for unregistered key --- | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when incrementing long, then throws StatisticNotRegisteredException") | ||
| void incrementLong_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.incrementLong(key("test", "nonexistent"), 1L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when incrementing int, then throws StatisticNotRegisteredException") | ||
| void incrementInt_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.incrementInt(key("test", "nonexistent"), 1)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when incrementing double, then throws StatisticNotRegisteredException") | ||
| void incrementDouble_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.incrementDouble(key("test", "nonexistent"), 1.0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting max long, then throws StatisticNotRegisteredException") | ||
| void setMaxLong_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setMaxLong(key("test", "nonexistent"), 1L)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting max int, then throws StatisticNotRegisteredException") | ||
| void setMaxInt_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setMaxInt(key("test", "nonexistent"), 1)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting max double, then throws StatisticNotRegisteredException") | ||
| void setMaxDouble_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setMaxDouble(key("test", "nonexistent"), 1.0)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when setting timestamp if absent, then throws StatisticNotRegisteredException") | ||
| void setTimestampIfAbsent_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.setTimestampIfAbsent(key("test", "nonexistent"), Instant.now())); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when adding to set, then throws StatisticNotRegisteredException") | ||
| void addToSet_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.addToSet(key("test", "nonexistent"), "value")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an unregistered key, when removing from set, then throws StatisticNotRegisteredException") | ||
| void removeFromSet_throws_forUnregisteredKey() { | ||
| assertThrows(StatisticNotRegisteredException.class, | ||
| () -> data.removeFromSet(key("test", "nonexistent"), "value")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add missing null/negative/max boundary tests for this suite.
This suite covers many happy/error paths, but it still misses required edge cases for null input and negative/max numeric boundaries on new test coverage (for example increment*, setMax*, and set operations).
As per coding guidelines, "Cover edge cases in tests: null inputs, empty collections, zero/negative numeric inputs, and max/limit values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/statistic/PlayerStatisticDataAdditionalTest.java`
around lines 89 - 412, Add unit tests to PlayerStatisticDataAdditionalTest
covering null and boundary numeric inputs: (1) verify null NamespacedKey or null
value passed to incrementInt/incrementLong/incrementDouble,
setMaxInt/setMaxLong/setMaxDouble, setTimestampIfAbsent, addToSet, and
removeFromSet produce the expected exception (e.g., NullPointerException or
IllegalArgumentException) by calling incrementInt/incrementLong/incrementDouble,
setMaxInt/setMaxLong/setMaxDouble, setTimestampIfAbsent, addToSet, removeFromSet
with null key or null element; (2) add tests for zero, negative, and max values
for incrementInt/incrementLong/incrementDouble and setMax* to assert correct
result or thrown exception (use
Integer.MAX_VALUE/Long.MAX_VALUE/Double.MAX_VALUE where appropriate); and (3)
add tests for empty and null collections when populating via populateFromEntries
and when adding/removing from set to ensure addToSet/removeFromSet behave
correctly. Use the existing helper keys (INT_KEY, LONG_KEY, DOUBLE_KEY,
TIMESTAMP_KEY, SET_KEY) and methods like incrementInt, incrementLong,
incrementDouble, setMaxInt, setMaxLong, setMaxDouble, setTimestampIfAbsent,
addToSet, removeFromSet, and populateFromEntries to locate targets.
Source: Coding guidelines
Comment on lines
+337
to
+347
| @Test | ||
| @DisplayName("Given a dirty key whose value was cleared, when getting modified entries, then that key is skipped") | ||
| void getModifiedEntries_skipsKey_whenValueIsNull() { | ||
| data.setValue(INT_KEY, 42); | ||
| assertTrue(data.isDirty()); | ||
| data.populateFromEntries(Map.of()); | ||
| data.setValue(INT_KEY, 99); | ||
| Map<NamespacedKey, StatisticEntry> modified = data.getModifiedEntries(); | ||
| assertTrue(modified.containsKey(INT_KEY)); | ||
| assertEquals(99, modified.get(INT_KEY).getAsInt()); | ||
| } |
There was a problem hiding this comment.
Test intent and assertions are inconsistent for null-skip behavior.
Line 338 says the key should be skipped when value is null, but Lines 345-346 assert the key is present with value 99. This currently validates overwrite behavior, not null-skip filtering in getModifiedEntries().
Suggested fix
`@Test`
`@DisplayName`("Given a dirty key whose value was cleared, when getting modified entries, then that key is skipped")
void getModifiedEntries_skipsKey_whenValueIsNull() {
data.setValue(INT_KEY, 42);
assertTrue(data.isDirty());
- data.populateFromEntries(Map.of());
- data.setValue(INT_KEY, 99);
- Map<NamespacedKey, StatisticEntry> modified = data.getModifiedEntries();
- assertTrue(modified.containsKey(INT_KEY));
- assertEquals(99, modified.get(INT_KEY).getAsInt());
+ data.populateFromEntries(Map.of( // clear backing value for dirty key
+ INT_KEY, new StatisticEntry(INT_KEY, StatisticType.INT, null)
+ ));
+ Map<NamespacedKey, StatisticEntry> modified = data.getModifiedEntries();
+ assertFalse(modified.containsKey(INT_KEY));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/statistic/PlayerStatisticDataAdditionalTest.java`
around lines 337 - 347, The test's assertions contradict its description: it
intends to verify that getModifiedEntries() skips a key whose value was
cleared/null, but it currently sets INT_KEY back to 99 and asserts presence;
update the test to actually clear the key (e.g., call the method that clears the
value or setValue(INT_KEY, null) after populateFromEntries(Map.of())) and then
call getModifiedEntries() and assert that the resulting Map does NOT contain
INT_KEY; reference the existing helpers and symbols data.setValue,
data.populateFromEntries, data.getModifiedEntries, INT_KEY, and StatisticEntry
when making this change.
Comment on lines
+35
to
+134
| @Test | ||
| @DisplayName("Given a single filter, when filtering, then applies that filter correctly") | ||
| void filter_appliesSingleFilter_correctly() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = new ChainPlayerContextFilter<>(evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6)); | ||
| assertEquals(List.of(2, 4, 6), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given two filters, when filtering, then applies both in order") | ||
| void filter_appliesMultipleFilters_inOrder() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> greaterThanThree = (player, list) -> | ||
| list.stream().filter(i -> i > 3).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(evenOnly, greaterThanThree); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6, 7, 8)); | ||
| assertEquals(List.of(4, 6, 8), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given two filters in reverse order, when filtering, then order matters") | ||
| void filter_orderMatters_forFilterChain() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> greaterThanThree = (player, list) -> | ||
| list.stream().filter(i -> i > 3).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(greaterThanThree, evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6, 7, 8)); | ||
| assertEquals(List.of(4, 6, 8), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an empty input collection, when filtering, then returns empty collection") | ||
| void filter_returnsEmpty_whenInputIsEmpty() { | ||
| PlayerContextFilter<String, TestCorePlayer> noopFilter = (player, list) -> list; | ||
|
|
||
| ChainPlayerContextFilter<String, TestCorePlayer> chain = new ChainPlayerContextFilter<>(noopFilter); | ||
|
|
||
| Collection<String> result = chain.filter(testPlayer, List.of()); | ||
| assertTrue(result.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a filter that reduces to empty, when chaining with another, then result is empty") | ||
| void filter_returnsEmpty_whenFirstFilterRemovesAll() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> removeAll = (player, list) -> List.of(); | ||
| PlayerContextFilter<Integer, TestCorePlayer> identity = (player, list) -> list; | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(removeAll, identity); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3)); | ||
| assertTrue(result.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given three chained filters, when filtering, then all three are applied sequentially") | ||
| void filter_appliesThreeFilters_sequentially() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> removeNegatives = (player, list) -> | ||
| list.stream().filter(i -> i >= 0).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> removeLargeThan10 = (player, list) -> | ||
| list.stream().filter(i -> i <= 10).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(removeNegatives, removeLargeThan10, evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(-5, -2, 0, 1, 2, 3, 4, 8, 11, 20)); | ||
| assertEquals(List.of(0, 2, 4, 8), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given string filters, when filtering with player context, then player object is accessible") | ||
| void filter_passesPlayerContext_toEachFilter() { | ||
| PlayerContextFilter<String, TestCorePlayer> filterByPlayerUUID = (player, list) -> | ||
| list.stream().filter(s -> !s.equals(player.getUUID().toString())).collect(Collectors.toList()); | ||
|
|
||
| ChainPlayerContextFilter<String, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(filterByPlayerUUID); | ||
|
|
||
| String playerUuidString = PLAYER_UUID.toString(); | ||
| Collection<String> result = chain.filter(testPlayer, List.of("hello", playerUuidString, "world")); | ||
| assertEquals(List.of("hello", "world"), new ArrayList<>(result)); | ||
| } |
There was a problem hiding this comment.
Add a null-input edge-case test for filter(...).
This suite covers empty input but not null input. Add a regression asserting the expected behavior (e.g., exception contract) for null collections.
As per coding guidelines, "Cover edge cases in tests: null inputs, empty collections, zero/negative numeric inputs, and max/limit values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/util/filter/ChainPlayerContextFilterTest.java`
around lines 35 - 134, Add a test method (e.g., filter_throwsWhenInputIsNull) in
ChainPlayerContextFilterTest that constructs a ChainPlayerContextFilter (use a
noop PlayerContextFilter or existing noopFilter) and asserts that calling
chain.filter(testPlayer, null) throws the expected exception (use
assertThrows(NullPointerException.class, ...) to verify the null-input
contract). Reference ChainPlayerContextFilter.filter and the test fixture
testPlayer when adding the assertion.
Source: Coding guidelines
Comment on lines
+37
to
+124
| void filter_appliesSingleFilter_correctly() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = new ChainPlayerContextFilter<>(evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6)); | ||
| assertEquals(List.of(2, 4, 6), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given two filters, when filtering, then applies both in order") | ||
| void filter_appliesMultipleFilters_inOrder() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> greaterThanThree = (player, list) -> | ||
| list.stream().filter(i -> i > 3).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(evenOnly, greaterThanThree); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6, 7, 8)); | ||
| assertEquals(List.of(4, 6, 8), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given two filters in reverse order, when filtering, then order matters") | ||
| void filter_orderMatters_forFilterChain() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> greaterThanThree = (player, list) -> | ||
| list.stream().filter(i -> i > 3).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(greaterThanThree, evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6, 7, 8)); | ||
| assertEquals(List.of(4, 6, 8), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given an empty input collection, when filtering, then returns empty collection") | ||
| void filter_returnsEmpty_whenInputIsEmpty() { | ||
| PlayerContextFilter<String, TestCorePlayer> noopFilter = (player, list) -> list; | ||
|
|
||
| ChainPlayerContextFilter<String, TestCorePlayer> chain = new ChainPlayerContextFilter<>(noopFilter); | ||
|
|
||
| Collection<String> result = chain.filter(testPlayer, List.of()); | ||
| assertTrue(result.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given a filter that reduces to empty, when chaining with another, then result is empty") | ||
| void filter_returnsEmpty_whenFirstFilterRemovesAll() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> removeAll = (player, list) -> List.of(); | ||
| PlayerContextFilter<Integer, TestCorePlayer> identity = (player, list) -> list; | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(removeAll, identity); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3)); | ||
| assertTrue(result.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given three chained filters, when filtering, then all three are applied sequentially") | ||
| void filter_appliesThreeFilters_sequentially() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> removeNegatives = (player, list) -> | ||
| list.stream().filter(i -> i >= 0).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> removeLargeThan10 = (player, list) -> | ||
| list.stream().filter(i -> i <= 10).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(removeNegatives, removeLargeThan10, evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(-5, -2, 0, 1, 2, 3, 4, 8, 11, 20)); | ||
| assertEquals(List.of(0, 2, 4, 8), new ArrayList<>(result)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Given string filters, when filtering with player context, then player object is accessible") | ||
| void filter_passesPlayerContext_toEachFilter() { |
There was a problem hiding this comment.
Test method names don’t follow the required givenContext_whenAction_thenOutcome convention.
Rename test methods to the mandated pattern for consistency and discoverability in this test suite.
As per coding guidelines, "Follow the givenContext_whenAction_thenOutcome naming convention for test methods (e.g., givenEmptyPage_whenGetSlots_thenReturnsEmpty)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/util/filter/ChainPlayerContextFilterTest.java`
around lines 37 - 124, The test method names do not follow the required
givenContext_whenAction_thenOutcome convention; rename each test method to that
pattern (e.g., filter_appliesSingleFilter_correctly ->
givenSingleFilter_whenFiltering_thenAppliesCorrectly,
filter_appliesMultipleFilters_inOrder ->
givenTwoFilters_whenFiltering_thenAppliesInOrder,
filter_orderMatters_forFilterChain ->
givenFiltersInReverseOrder_whenFiltering_thenOrderMatters,
filter_returnsEmpty_whenInputIsEmpty ->
givenEmptyInput_whenFiltering_thenReturnsEmpty,
filter_returnsEmpty_whenFirstFilterRemovesAll ->
givenFirstFilterRemovesAll_whenChaining_thenReturnsEmpty,
filter_appliesThreeFilters_sequentially ->
givenThreeFilters_whenFiltering_thenAllAppliedSequentially,
filter_passesPlayerContext_toEachFilter ->
givenStringFilters_whenFilteringWithPlayerContext_thenPlayerAccessible); update
the method names in ChainPlayerContextFilterTest accordingly and run tests to
ensure no references (e.g., `@DisplayName` remains) are broken.
Source: Coding guidelines
Comment on lines
+64
to
+77
| @DisplayName("Given two filters in reverse order, when filtering, then order matters") | ||
| void filter_orderMatters_forFilterChain() { | ||
| PlayerContextFilter<Integer, TestCorePlayer> greaterThanThree = (player, list) -> | ||
| list.stream().filter(i -> i > 3).collect(Collectors.toList()); | ||
| PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) -> | ||
| list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList()); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| ChainPlayerContextFilter<Integer, TestCorePlayer> chain = | ||
| new ChainPlayerContextFilter<>(greaterThanThree, evenOnly); | ||
|
|
||
| Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6, 7, 8)); | ||
| assertEquals(List.of(4, 6, 8), new ArrayList<>(result)); | ||
| } |
There was a problem hiding this comment.
“Order matters” test currently does not prove order sensitivity.
Both filters are simple predicates (> 3 and even), so applying them in either order yields the same result. This test can pass even if chaining order is broken.
Proposed fix
`@Test`
`@DisplayName`("Given two filters in reverse order, when filtering, then order matters")
void filter_orderMatters_forFilterChain() {
- PlayerContextFilter<Integer, TestCorePlayer> greaterThanThree = (player, list) ->
- list.stream().filter(i -> i > 3).collect(Collectors.toList());
- PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) ->
- list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList());
+ PlayerContextFilter<Integer, TestCorePlayer> takeFirstThree = (player, list) ->
+ list.stream().limit(3).collect(Collectors.toList());
+ PlayerContextFilter<Integer, TestCorePlayer> evenOnly = (player, list) ->
+ list.stream().filter(i -> i % 2 == 0).collect(Collectors.toList());
`@SuppressWarnings`("unchecked")
ChainPlayerContextFilter<Integer, TestCorePlayer> chain =
- new ChainPlayerContextFilter<>(greaterThanThree, evenOnly);
+ new ChainPlayerContextFilter<>(takeFirstThree, evenOnly);
- Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6, 7, 8));
- assertEquals(List.of(4, 6, 8), new ArrayList<>(result));
+ Collection<Integer> result = chain.filter(testPlayer, List.of(1, 2, 3, 4, 5, 6));
+ assertEquals(List.of(2), new ArrayList<>(result));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/util/filter/ChainPlayerContextFilterTest.java`
around lines 64 - 77, The test "filter_orderMatters_forFilterChain" currently
uses two commutative predicates (greaterThanThree and evenOnly) so it doesn't
prove order sensitivity; replace the filters with order-dependent ones (for
example a take-first-N filter and an even-only filter) so chaining order changes
the result: implement something like takeFirstThree = (player,list) ->
list.stream().limit(3).collect(...) and keep evenOnly = (player,list) ->
list.stream().filter(i -> i % 2 == 0).collect(...), create the
ChainPlayerContextFilter with (takeFirstThree, evenOnly), call
chain.filter(testPlayer, List.of(...)) and assert the expected different result
(e.g. when using List.of(1,2,3,4,5,6) expect [2] for takeFirstThree then
evenOnly but a different list if reversed), updating the assertion accordingly;
reference ChainPlayerContextFilter, filter_orderMatters_forFilterChain,
takeFirstThree, evenOnly, and the filter(...) method to locate and change the
code.
Comment on lines
+138
to
+140
| TestCorePlayer(UUID uuid) { | ||
| super(uuid, null); | ||
| } |
There was a problem hiding this comment.
Annotate the non-null constructor parameter with @NotNull.
TestCorePlayer(UUID uuid) receives a non-null UUID in this test and should carry the required nullability annotation.
As per coding guidelines, "Add @NotNull annotation from IntelliJ annotations v12 on all non-null return types and parameters."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/diamonddagger590/mccore/util/filter/ChainPlayerContextFilterTest.java`
around lines 138 - 140, The TestCorePlayer(UUID uuid) constructor parameter
should be annotated with `@NotNull` (IntelliJ annotations v12); update the
constructor signature in class TestCorePlayer to import and apply
org.jetbrains.annotations.NotNull to the uuid parameter so the non-null contract
is explicit and matches project guidelines.
Source: Coding guidelines
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
getUUID(),getValue()(raw Object accessor for all types),bulkIncrementLong()delegation and accumulation,getOrCreateSetwith plainHashSet(non-LinkedHashSet) populated viapopulateFromEntries, cancellation paths forincrementInt,incrementDouble,incrementLong,setMaxLong,setMaxInt,setMaxDouble,setTimestampIfAbsent,addToSet,removeFromSet, andStatisticNotRegisteredExceptionfor all mutators (90.3% → 96.7% line coverage)fromName()for all known names (nexo, itemsadder, items_adder, none), case-insensitive matching, unrecognized name fallback to NONE via parameterized test, and enum completeness (0% → 23.6% line coverage; remaining 76% are CorePlugin-dependent lambda bodies)Classes covered (avoiding PR #27, PR #28, PR #29, PR #30, PR #31, and PR #32 overlap)
PR #27 covers GetItemRequest, StatisticEntry, ReloadableContent, ReloadableContentManager, StartupProfile. PR #28 covers Methods, PlayerSettingRegistry, ReloadableContent subtypes. PR #29 covers Mutexable, ParseError, EvaluationException, and exception classes. PR #30 covers RegistryAccess, StatisticModifyEvent, PostStatisticModifyEvent, Transaction. PR #31 covers database events, GUI events, SQLiteDatabaseDriver, DatabaseDriver, ManagerKey/PluginHookKey constants. PR #32 covers player events, CorePlayer, Credentials, ConnectionDetails, CorePlayerOfflineException. This PR targets entirely different classes with no overlap.
Coverage impact
4 classes improved: 2 brought from 0% to 100%, 1 brought from 0% to 23.6%, 1 improved from 90.3% to 96.7%. Overall project coverage: 27.0% → 28.3%.
Test plan
./gradlew testhttps://claude.ai/code/session_012thebA7utDjU8bJZqBUAB9
Generated by Claude Code
Summary by CodeRabbit