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 flaky test shouldGetClsAsRootEntity in SubjectClosureResolver_TestCase.java #52

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

Conversation

nikunjagarwal321
Copy link

What changes were proposed in this pull request?

Changing the assertion from contains() to hasItems() in the testcase : shouldGetClsAsRootEntity in class SubjectClosureResolver_TestCase.

Why are the changes needed?

Multiple flaky tests were detected in the above testcase while trying to run the tests using the nondex tool. NonDex is a tool for detecting and debugging wrong assumptions on under-determined Java APIs.

Steps to reproduce flakiness using nondex -

mvn install

mvn test -Dtest=edu.stanford.protege.webprotege.entity.SubjectClosureResolver_TestCase#shouldGetClsAsRootEntity

mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=edu.stanford.protege.webprotege.entity.SubjectClosureResolver_TestCase#shouldGetClsAsRootEntity

ERROR logs:

[ERROR]   SubjectClosureResolver_TestCase.shouldGetClsAsRootEntity:71 
Expected: iterable containing [<<http://example.org/Cls>>, <<http://example.org/Other>>]
     but: item 0: was <<http://example.org/Other>>
[INFO] 
Screenshot 2024-11-09 at 3 04 16 PM

Reason for Failure

The method shouldGetClsAsRootEntity uses toSet() method which has no guarantee of order.

Screenshot 2024-11-09 at 3 06 29 PM

The contains() method also checks the order of the entities which are being compared. It checks for the order of the two objects cls and valueEntity as well. This can lead to failure of test case when valueEntity comes after cls. Hence the proposed change is to use method hasItems() which checks if the entities are present in the set or not, irrespective of the order.

Please let me know if you have any questions or would like to discuss this further for any clarificaitions.

…estCase by make it deterministic and free from order
@nikunjagarwal321
Copy link
Author

Thanks for reviewing and approving the changes @alexsilaghi.

If the changes look good, can we go ahead and merge them as well? Or are there some pending action items from my side?

@nikunjagarwal321
Copy link
Author

Hi @alexsilaghi following up on this. Since the changes look good, can we go ahead and merge the PR?

@nikunjagarwal321
Copy link
Author

++ @tudorache for review

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.

2 participants