Skip to content

Fix flaky tests: JSON comparison#2718

Merged
bjagg merged 2 commits into
uPortal-Project:masterfrom
KiruthikaJanakiraman:fix-flaky
Nov 7, 2023
Merged

Fix flaky tests: JSON comparison#2718
bjagg merged 2 commits into
uPortal-Project:masterfrom
KiruthikaJanakiraman:fix-flaky

Conversation

@KiruthikaJanakiraman

@KiruthikaJanakiraman KiruthikaJanakiraman commented Nov 7, 2023

Copy link
Copy Markdown
Contributor
Checklist
Description of change

This PR aims to fix the following 5 flaky test:

  1. org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsActorSerialize
  2. org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsObjectSerialize
  3. org.apereo.portal.events.AnalyticsIncorporationComponentEventSerializationTest.testMixinNoCopy
  4. org.apereo.portal.events.AnalyticsIncorporationComponentEventSerializationTest.testEventFiltering
  5. org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsStatementSerialize

I found and confirmed the flaky behavior of the tests using an open-source research tool NonDex, which shuffles implementations of nondeterminism operations.

Problem
The test case org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsActorSerialize fails due to the below assertion as the order of the fields within the object lrsActor changes upon nondeterministic shuffling causing the order of JSON string result to change.

uPortal/uPortal-webapp/src/test/java/org/apereo/portal/events/tincan/json/LrsDataModelSerializeTest.java

Lines 52 to 57 in 59f8895

final String result = this.objectMapper.writeValueAsString(lrsActor);
assertEquals(
"{\"mbox\":\"user@example.com\",\"name\":\"John Doe\",\"objectType\":\"Agent\"}",
result);
}

Solution
This PR fixes the flakiness by using an ObjectMapper class to convert the JSON strings to JsonNode and then comparing the JsonNodes using JsonNode.equals method which performs a full(deep) comparison.

The other 4 test are also fixed similarly using this method.

Steps to reproduce
The following command can be used to reproduce the assertion errors and verify the fix.

Integrate NonDex:

Add the following snippet to the top of the build.gradle in $PROJ_DIR:

plugins {
    id 'edu.illinois.nondex' version '2.1.1-1'
}

Append to build.gradle in $PROJ_DIR:

apply plugin: 'edu.illinois.nondex'

Execute Test with Gradle:

./gradlew --info uPortal-webapp:test --tests org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsActorSerialize
./gradlew --info uPortal-webapp:test --tests org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsObjectSerialize
./gradlew --info uPortal-webapp:test --tests org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsStatementSerialize
./gradlew --info uPortal-webapp:test --tests org.apereo.portal.events.AnalyticsIncorporationComponentEventSerializationTest.testEventFiltering
./gradlew --info uPortal-webapp:test --tests org.apereo.portal.events.AnalyticsIncorporationComponentEventSerializationTest.testMixinNoCopy

Run NonDex:

./gradlew --info uPortal-webapp:nondexTest --tests=org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsActorSerialize --nondexRuns=50 -x autoLintGradle
./gradlew --info uPortal-webapp:nondexTest --tests=org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsObjectSerialize --nondexRuns=50 -x autoLintGradle
./gradlew --info uPortal-webapp:nondexTest --tests=org.apereo.portal.events.tincan.json.LrsDataModelSerializeTest.testLrsStatementSerialize --nondexRuns=50 -x autoLintGradle
./gradlew --info uPortal-webapp:nondexTest --tests=org.apereo.portal.events.AnalyticsIncorporationComponentEventSerializationTest.testEventFiltering --nondexRuns=50 -x autoLintGradle
./gradlew --info uPortal-webapp:nondexTest --tests=org.apereo.portal.events.AnalyticsIncorporationComponentEventSerializationTest.testMixinNoCopy --nondexRuns=50 -x autoLintGradle

Test Environment:

Java version "1.8.0_381"
macOS Venture Version 13.4.1 (22F82)

Please let me know if you have any concerns or questions.

@ChristianMurphy

ChristianMurphy commented Nov 7, 2023

Copy link
Copy Markdown
Member

Welcome @KiruthikaJanakiraman! 👋
Thanks for taking the time to contribute!

  1. When you have a chance, could you submit the Apereo ICLA? https://www.apereo.org/licensing/agreements/icla
  2. The tests are currently failing due to formatting, could you run ./gradlew goJF and commit the updated files?
  3. To confirm your goal, your goal is to reduce flakiness in CI/CD? (Very much appreciated) Or is there also a bug you are trying to reproduce?

@KiruthikaJanakiraman

KiruthikaJanakiraman commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

Welcome @KiruthikaJanakiraman! 👋 Thanks for taking the time to contribute!

  1. When you have a chance, could you submit the Apereo ICLA? https://www.apereo.org/licensing/agreements/icla
  2. The tests are currently failing due to formatting, could you run ./gradlew goJF and commit the updated files?
  3. To confirm your goal, your goal is to reduce flakiness in CI/CD? (Very much appreciated) Or is there also a bug you are trying to reproduce?

Hi Christian,

  1. I have submitted the ICLA.
  2. I ran the given command(./gradlew goJF) and committed the formatted files.
  3. My goal here is to remove the flakiness from test cases which I identified using the tool NonDex. NonDex explores different behaviors of under-determined APIs and reports test failures under different explored behaviors.

ChristianMurphy
ChristianMurphy approved these changes Nov 7, 2023
View reviewed changes

@ChristianMurphy ChristianMurphy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @KiruthikaJanakiraman! LGTM

@bjagg bjagg merged commit 989ba28 into uPortal-Project:master Nov 7, 2023
This was referenced Nov 9, 2023
Merged
Merged
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjagg bjagg bjagg approved these changes

@ChristianMurphy ChristianMurphy ChristianMurphy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KiruthikaJanakiraman @ChristianMurphy @bjagg