SAK-52163 Fix flaky tests across multiple modules #14230
Conversation
WalkthroughUpdated tests to use structural and deterministic checks: JSON comparisons now parse and compare JSON trees; JWKS tests validate each key's Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (2)
lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java (1)
190-205: Consider extracting validation logic to reduce duplication.The same validation pattern (parse JSON, validate keys array, check field presence) is repeated from lines 164-179. Extracting this into a helper method would improve maintainability.
Example helper method:
private void validateKeySetJSON(String keySetJSON, int expectedSize) { JSONObject ks_json = LTIUtil.parseJSONObject(keySetJSON); assertNotNull(ks_json); JSONArray keys = (JSONArray) ks_json.get("keys"); assertNotNull(keys); assertEquals(keys.size(), expectedSize); for(int i=0; i< keys.size(); i++) { JSONObject key = (JSONObject) keys.get(i); assertNotNull(key); assertNotNull(key.get("kid")); assertNotNull(key.get("n")); assertNotNull(key.get("kty")); assertNotNull(key.get("alg")); } }Then replace both validation blocks with:
- JSONObject ks_json = LTIUtil.parseJSONObject(keySetJSON); - assertNotNull(ks_json); - - JSONArray keys = (JSONArray) ks_json.get("keys"); - assertNotNull(keys); - assertEquals(keys.size(), 3); - - for(int i=0; i< keys.size(); i++) { - JSONObject key = (JSONObject) keys.get(i); - assertNotNull(key); - assertNotNull(key.get("kid")); - assertNotNull(key.get("n")); - assertNotNull(key.get("kty")); - assertNotNull(key.get("alg")); - } + validateKeySetJSON(keySetJSON, 3);rwiki/rwiki-impl/impl/src/test/uk/ac/cam/caret/sakai/rwiki/component/service/impl/test/RWikiObjectServiceImplTest.java (1)
86-86: Excellent fix for nondeterministic test behavior.Switching to LinkedHashSet ensures deterministic iteration order, which is critical for this test to reliably validate truncation behavior when references exceed the length limit.
Optional improvement: Consider declaring the variable as
LinkedHashSet<String>instead ofHashSet<String>to make the insertion-order guarantee explicit and document why this specific implementation is required.Apply this diff if you want to make the intent more explicit:
- HashSet<String> hs = new LinkedHashSet<>(); + LinkedHashSet<String> hs = new LinkedHashSet<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java(2 hunks)lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.java(2 hunks)rwiki/rwiki-impl/impl/src/test/uk/ac/cam/caret/sakai/rwiki/component/service/impl/test/RWikiObjectServiceImplTest.java(2 hunks)samigo/samigo-services/src/test/org/sakaiproject/tool/assessment/services/ItemCancellationTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.
Files:
lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.javasamigo/samigo-services/src/test/org/sakaiproject/tool/assessment/services/ItemCancellationTest.javarwiki/rwiki-impl/impl/src/test/uk/ac/cam/caret/sakai/rwiki/component/service/impl/test/RWikiObjectServiceImplTest.javalti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Include tests where sensible/possible
🔇 Additional comments (6)
lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java (1)
164-179: Excellent fix for the flaky test!The structural JSON validation correctly addresses the nondeterministic field ordering in JSONObject. Parsing the JSON and explicitly checking for required fields (
kid,n,kty,alg) makes the test robust against field-order variations.rwiki/rwiki-impl/impl/src/test/uk/ac/cam/caret/sakai/rwiki/component/service/impl/test/RWikiObjectServiceImplTest.java (1)
26-26: LGTM: Import addition is necessary and correct.The LinkedHashSet import is required for the deterministic test fix below.
lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.java (2)
22-22: LGTM! Import added to support structural JSON comparison.The JsonNode import is correctly added to enable parsing JSON strings into tree structures for order-independent comparison.
247-252: LGTM! Structural JSON comparison eliminates field-order dependency.The test now parses both prettyPrintLog() outputs as JSON trees and compares them structurally. This correctly addresses the flaky test issue caused by non-deterministic JSON field ordering in Jackson serialization.
samigo/samigo-services/src/test/org/sakaiproject/tool/assessment/services/ItemCancellationTest.java (2)
28-28: LGTM! Import added to support deterministic sorting.The
Comparatorimport is necessary for the sorting logic introduced to fix flaky tests.
216-220: Excellent fix for the flaky test issue!The sorting logic correctly addresses the nondeterministic ordering from HashMap iteration by ensuring both arrays are compared in a consistent itemId-based order. The implementation is clear and properly commented.
Note: The current test data uses sequential IDs (0L, 1L, 2L...) that match array indices, making the
cancelItemIndexaccess pattern at line 226 safe after sorting. If test data evolves to use non-sequential IDs, consider looking up the cancelled item by ID rather than by index.
|
@lycoris106 please request access to JIRA at https://sakaiproject.atlassian.net/ be sure to include info about yourself and if your related to any higher ed organization to help speed up the approval |
lycoris106
force-pushed
the
fix-flaky-tests
branch
from
685a11e to
369f134
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java(2 hunks)lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.java(2 hunks)rwiki/rwiki-impl/impl/src/test/uk/ac/cam/caret/sakai/rwiki/component/service/impl/test/RWikiObjectServiceImplTest.java(2 hunks)samigo/samigo-services/src/test/org/sakaiproject/tool/assessment/services/ItemCancellationTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- samigo/samigo-services/src/test/org/sakaiproject/tool/assessment/services/ItemCancellationTest.java
- rwiki/rwiki-impl/impl/src/test/uk/ac/cam/caret/sakai/rwiki/component/service/impl/test/RWikiObjectServiceImplTest.java
- lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.
Files:
lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Include tests where sensible/possible
| String keySetJSON = SakaiKeySetUtil.getKeySet(); | ||
| assertNotNull(keySetJSON); | ||
| assertTrue(keySetJSON.contains("{\"keys\":[{\"kty\":")); | ||
| assertTrue(keySetJSON.contains(",\"alg\":\"RS256\"")); | ||
| JSONObject ks_json = LTIUtil.parseJSONObject(keySetJSON); | ||
| JSONArray keys = (JSONArray) ks_json.get("keys"); | ||
| assertEquals(keys.size(), 3); | ||
|
|
||
| for(int i=0; i< keys.size(); i++) { | ||
| JSONObject key = (JSONObject) keys.get(i); | ||
| assertNotNull(key); | ||
| String kid = (String) key.get("kid"); | ||
| assertNotNull(kid); | ||
| String n = (String) key.get("n"); | ||
| assertNotNull(n); | ||
| assertNotNull(key.get("kid")); | ||
| assertNotNull(key.get("n")); | ||
| assertNotNull(key.get("kty")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen structural validation and add missing null checks.
The transition from string-based to structural JSON validation is the right approach to fix flaky tests. However, there are gaps:
- Missing null checks after parsing:
ks_json(line 164) andkeys(line 165) should be validated withassertNotNullbefore use. - Inconsistent field validation: the
algfield is still checked via string assertion (line 163), whilekid,n, andktyare checked structurally in the loop. For consistency and completeness, includealgin the loop validation.
Apply this diff to add null checks and complete the structural validation:
String keySetJSON = SakaiKeySetUtil.getKeySet();
-assertTrue(keySetJSON.contains(",\"alg\":\"RS256\""));
JSONObject ks_json = LTIUtil.parseJSONObject(keySetJSON);
+assertNotNull(ks_json);
JSONArray keys = (JSONArray) ks_json.get("keys");
+assertNotNull(keys);
assertEquals(keys.size(), 3);
for(int i=0; i< keys.size(); i++) {
JSONObject key = (JSONObject) keys.get(i);
assertNotNull(key);
assertNotNull(key.get("kid"));
assertNotNull(key.get("n"));
assertNotNull(key.get("kty"));
+ assertNotNull(key.get("alg"));
+ assertEquals("RS256", key.get("alg"));
}🤖 Prompt for AI Agents
In lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java
around lines 162 to 174, add null checks after parsing and move/extend the alg
validation into the structural loop: assertNotNull(ks_json) and
assertNotNull(keys) immediately after parsing, and in the loop validate that
each key JSONObject contains non-null entries for "kid", "n", "kty", and "alg"
(remove the separate string contains assertion), keeping the existing size
assertion for keys.
| keySetJSON = SakaiKeySetUtil.getKeySet(); | ||
| assertNotNull(keySetJSON); | ||
| assertTrue(keySetJSON.contains("{\"keys\":[{\"kty\":")); | ||
| assertTrue(keySetJSON.contains(",\"alg\":\"RS256\"")); | ||
| ks_json = LTIUtil.parseJSONObject(keySetJSON); | ||
| ks_json = LTIUtil.parseJSONObject(keySetJSON); | ||
| keys = (JSONArray) ks_json.get("keys"); | ||
| assertEquals(keys.size(), 2); | ||
|
|
||
| for(int i=0; i< keys.size(); i++) { | ||
| JSONObject key = (JSONObject) keys.get(i); | ||
| assertNotNull(key); | ||
| assertNotNull(key.get("kid")); | ||
| assertNotNull(key.get("n")); | ||
| assertNotNull(key.get("kty")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply the same structural validation improvements here.
This section has the same gaps as the earlier validation block:
- Missing null checks after parsing:
ks_json(line 185) andkeys(line 186) should be validated withassertNotNull. - Inconsistent field validation: the
algfield is checked via string assertion (line 184) instead of structurally in the loop.
Apply this diff for consistency:
keySetJSON = SakaiKeySetUtil.getKeySet();
-assertTrue(keySetJSON.contains(",\"alg\":\"RS256\""));
ks_json = LTIUtil.parseJSONObject(keySetJSON);
+assertNotNull(ks_json);
keys = (JSONArray) ks_json.get("keys");
+assertNotNull(keys);
assertEquals(keys.size(), 2);
for(int i=0; i< keys.size(); i++) {
JSONObject key = (JSONObject) keys.get(i);
assertNotNull(key);
assertNotNull(key.get("kid"));
assertNotNull(key.get("n"));
assertNotNull(key.get("kty"));
+ assertNotNull(key.get("alg"));
+ assertEquals("RS256", key.get("alg"));
}🤖 Prompt for AI Agents
In lti/lti-common/src/test/org/sakaiproject/lti/util/SakaiKeySetUtilTest.java
around lines 183 to 195, add structural null checks for the parsed objects and
validate the alg field per-key: after parsing, assertNotNull(ks_json) and
assertNotNull(keys) before asserting keys.size(), then inside the loop
assertNotNull(key.get("alg")) and assertEquals("RS256", key.get("alg")) (instead
of using the external string contains check), while retaining the existing
assertions for kid, n, and kty.
|
@ern Since I couldn't find any option to request access anywhere at https://sakaiproject.atlassian.net/, I have directly emailed [email protected] for access (my CLA has been recorded at https://licensing.apereo.org/). I also made a new commit just to remove some redundant lines of fix to make the review easier. |
lycoris106
changed the title
|
@ern I have created the issue on JIRA and changed the PR title. If there's anything else I can do please feel free to tell me! (Sorry I couldn't change the branch name w/o reopening a PR) |
lycoris106
force-pushed
the
fix-flaky-tests
branch
from
0b86718 to
7f9162a
Compare
|
This PR has been automatically marked as stale due to 21 days of inactivity. It will be closed in 7 days unless there is new activity. |
|
If you think only some of the fix is necessary, please feel free to tell me. |
|
can you resolve the coderabbitai suggestions |
2 participants
Summary
Fixes nondeterministic (flaky) test in following modules:
The failure was found with NonDex, which explores non-determinism in tests. These tests can cause test failure under different JVMs, etc.
Notes
My ICLA (submitted and emailed on Nov 9) have not been approved and I do not have JIRA permission now either, therefore the branch name and commit message does not contain the right JIRA reference number. I submitted this PR considering it might be better to have maintainers review the fix as soon as possible. This commit only modifies testing code, but not any implementation code under test.
Please let me know if any changes are needed for this PR or if I should take additional steps to obtain JIRA permissions.
Root Cause and Fix
rwiki/rwiki-impl/impl
RWikiCurrentObjectImpl, the fields in hashsethscould be iterated in differnt orders (longStringmay not be the first one iterated).lti/tsugi-util
The order of fields when using jackson Mapper is not guaranteed, as the
jackson doc describes:
JsonNodeobjects instead of raw strings, making tests independent of field order.lti/lti-common
samigo/samigo-services
Failure Reproduction
Summary by CodeRabbit