Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed 6 flaky tests caused by unordered non-deterministic data-structures in lti/tsugi-util #13014

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hermya
Copy link

@hermya hermya commented Nov 11, 2024

Fixed flaky test cases in the module: ./lti/tsugi-util/

Test-cases details

Sr Test-case Point of failure Fixed by
1 org.tsugi.lti13.GroupServiceTest# testConstructor The bean of GroupService is converted to Json, and its string representation is asserted against expected value. JSON conversion is non-deterministic, and the order of attributes can be different on different jvms, resulting in different string representations Converting the Json string to a Map and Asserting Map-equality, as it doesn't require matching key-ordering
2 org.tsugi.lti13.LTI13KeySetTest# testKeySets In class org.tsugi.lti13.LTI13KeySetUtil at Line:60, rsaKey is being converted to a json-string, used further in a JsonObject. As rsaKey is a bean, its JSON conversion can result in non-deterministic string Same reason as 1 Converting the Json string to a Map and Asserting Map-equality, and a flexible contains check for asserting goodness of keySetJSON
3 org.tsugi.lti13.LTI13NimbusTest# testRSAFromString, testRSAPaulGray Same reason as 2 for both test-cases Adding a flexible contains check for asserting goodness of keySetJSON
4 org.tsugi.lti13.LTI13ObjectTes# testTwo, testThree Same reason as 1 for both test-cases Same fix as 1 for both test-cases

Fixed using NonDex

Suggested command to check all flaky tests :

mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex

For the particular test class:

mvn -pl ./lti/tsugi-util edu.illinois:nondex-maven-plugin:2.1.7:nondex \
-DnondexMode=ONE \
-DnondexRuns=5  \
-Dtest=org.tsugi.lti13.GroupServiceTest#testConstructor

For more information: https://github.com/TestingResearchIllinois/NonDex

@hermya hermya force-pushed the fix-flaky branch 2 times, most recently from 5f15aa0 to a3745d0 Compare November 11, 2024 18:00
@csev
Copy link
Contributor

csev commented Nov 12, 2024

@hermya Thanks for the patch - it looks good to me. We need to file JIRA in our issue tracker and have you fill out a Contribution Agreement. If your intent is to keep working on Sakai it would be worthwhile to go through all this effort - but if this is a one-time patch, I can take this fix over for you and do all the necessary stuff to get this into the code base. Sorry about the paperwork - open source projects need to do this.

@hermya
Copy link
Author

hermya commented Nov 12, 2024

Hello @csev,
I'll be happy to fill out the CLA and continue to contribute in future too! Can you please suggest how to do that? I tried logging in on Jira but it shows that I don't have access to create a ticket

@ottenhoff
Copy link
Contributor

I believe there is a form linked from this page to create an individual CLA: https://form.jotform.com/60435795225156

@hermya
Copy link
Author

hermya commented Nov 12, 2024

Found it and submitted, thanks @ottenhoff!

@ottenhoff ottenhoff requested a review from csev November 15, 2024 17:34
@csev
Copy link
Contributor

csev commented Nov 16, 2024

@ottenhoff - The technical bits look good to me. Once this is merged, I want to apply the approach to some new tests I just wrote this weekend :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants