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

Fixing three flaky tests in AuthorisationsTest and ViewTest #3327

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Sitara-a
Copy link

@Sitara-a Sitara-a commented Oct 28, 2024

Two flaky tests were fixed in this pull request-

  1. uk.gov.gchq.gaffer.commonutil.elementvisibilityutil.AuthorisationsTest.testToString
  2. uk.gov.gchq.gaffer.data.elementdefinition.view.ViewTest.shouldCreateAnIdenticalObjectWhenCloned
  3. uk.gov.gchq.gaffer.data.elementdefinition.view.ViewUtilTest.shouldCreateAnIdenticalObjectWhenCloned

Flakiness in the tests -

To check for flakiness in the tests, a tool called nondex was used.

Steps to reproduce flakiness using nondex -

Test 1:

mvn install -pl core/common-util -am -DskipTests
mvn -pl core/common-util test -Dtest=uk.gov.gchq.gaffer.commonutil.elementvisibilityutil.AuthorisationsTest
mvn -pl core/common-util edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=uk.gov.gchq.gaffer.commonutil.elementvisibilityutil.AuthorisationsTest

The third command shows us the flakiness in the test. There are test failures due to differences in the order of elements
image

Test 2:

mvn install -pl core/data -am -DskipTests
mvn -pl core/data test -Dtest=uk.gov.gchq.gaffer.data.elementdefinition.view.ViewTest
mvn -pl core/data edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=uk.gov.gchq.gaffer.data.elementdefinition.view.ViewTest

The third command shows us the flakiness in the test. There are test failures due to differences in order of elements
image

Test 3:

mvn install -pl core/data -am -DskipTests
mvn -pl core/data test -Dtest=uk.gov.gchq.gaffer.data.elementdefinition.view.ViewUtilTest.shouldCreateAnIdenticalObjectWhenCloned
mvn -pl core/data edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=uk.gov.gchq.gaffer.data.elementdefinition.view.ViewUtilTest.shouldCreateAnIdenticalObjectWhenCloned

Source of Flakiness in tests and fixes:

  1. AuthorisationsTest.testToString
    The Authorisation object created in the test case appends three strings to the auths member of the class which is a HashSet. The toString() method of the class then iterates over this HashSet and builds a String after concatenating the members of the set. Since a Java HashSet does not store order, the test case fails if the elements of the HashSet are iterated over in any order other than the order in which they were appended during object instantiation. To fix this, the test was updated to check if all strings in the auths member of the object are a part of the String returned by toString() instead of focusing on the order.
image
  1. ViewTest.shouldCreateAnIdenticalObjectWhenCloned
    In this test case, the source of flakiness is the following lines -
    final byte[] viewJson = view.toCompactJson();
    final byte[] cloneJson = clone.toCompactJson();

assertThat(cloneJson).containsExactly(viewJson);

The method toCompactJson() internally uses JSONSerialiser.serialise() to convert the object into a byte stream. This does not preserve the order of attributes. Therefore, the containsExactly() method fails when the order of attributes differs in the two objects. To fix this, an ObjectMapper object was created and two JsonNode objects were created after calling the readTree() method on the two byte arrays. The equality check was then applied on these two JsonNode objects.

  1. ViewUtilTest.shouldCreateAnIdenticalObjectWhenCloned
    Similar to test 2, the source of flakiness is from the toCompactJson() method which does not preserve the order of attributes.

Please let me know if you have any questions or need any additional justification/changes from my side.

@CLAassistant
Copy link

CLAassistant commented Oct 28, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.94%. Comparing base (73b7544) to head (0bcd9cf).

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #3327   +/-   ##
==========================================
  Coverage      67.94%   67.94%           
  Complexity      2596     2596           
==========================================
  Files            955      955           
  Lines          30530    30530           
  Branches        3369     3369           
==========================================
  Hits           20744    20744           
  Misses          8306     8306           
  Partials        1480     1480           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@omkrpt
Copy link

omkrpt commented Nov 7, 2024

Looks good to me.

@omkrpt
Copy link

omkrpt commented Nov 7, 2024

Have fixed a couple more tests (#3155, #3156) do let me know if they make sense @Sitara-a

@Sitara-a Sitara-a changed the title Fixing two flaky tests in AuthorisationsTest and ViewTest Fixing three flaky tests in AuthorisationsTest and ViewTest Nov 18, 2024
@Sitara-a
Copy link
Author

Have fixed a couple more tests (#3155, #3156) do let me know if they make sense @Sitara-a

Yup! The fixes look good to me.

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

Successfully merging this pull request may close these issues.

3 participants