rRajivramachandran opened a new pull request, #8185: URL: https://github.com/apache/cloudstack/pull/8185
### Description This PR is to fix a few tests whose results are non deterministic/flaky in the module plugins/network-elements/nicira-nvp. ### Setup: Java version: 11.0.20.1 Maven version: 3.8.8 ### Test failure reproduction: The test `com.cloud.network.nicira.NiciraRestClientTest#testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect` assumes the order of keys to be maintained by a `HashMap`. This issue was verified using the [NonDex plugin](https://github.com/TestingResearchIllinois/NonDex). #### Steps: ``` git clone https://github.com/apache/cloudstack.git cd cloudstack mvn install -pl plugins/network-elements/nicira-nvp -am -DskipTests mvn -pl plugins/network-elements/nicira-nvp edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=com.cloud.network.nicira.NiciraRestClientTest#testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect ``` ### Root cause and fix: Test failure in NonDex mode: ``` [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.767 s <<< FAILURE! - in com.cloud.network.nicira.NiciraRestClientTest [ERROR] testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect(com.cloud.network.nicira.NiciraRestClientTest) Time elapsed: 1.761 s <<< ERROR! java.lang.NullPointerException ``` The NullPointerException occurs at the following location https://github.com/apache/cloudstack/blob/fcbf540369d5d51145573dc948e21930531a3fad/plugins/network-elements/nicira-nvp/src/main/java/com/cloud/network/nicira/NiciraRestClient.java#L87 because the `mockResponse` created in the test, does not get returned for the `execute` method. The `HttpRequestMatcher` serializes the request object to a string https://github.com/apache/cloudstack/blob/fcbf540369d5d51145573dc948e21930531a3fad/utils/src/test/java/com/cloud/utils/rest/HttpRequestMatcher.java#L68 To verify that the issue was due to the HttpRequestMatcher used by mockito, the following line: https://github.com/apache/cloudstack/blob/fcbf540369d5d51145573dc948e21930531a3fad/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraRestClientTest.java#L190 was modfied to read `when(httpClient.execute(eq(HTTP_HOST), any(HttpUriRequest.class), eq(httpClientContext)))`. This allows matching any request class object. The test passed with this change when run on the NonDex tool confirming that the issue was here. On adding logs to the `HttpRequestMatcher` class, it was deduced that the test fails when the order of keys returned by the login parameters is not the same as that configured in the test class. Failing case(ordered shuffled): ``` 2023-10-19 18:11:15,312 ERROR [root] (main:) Wanted payload:password=adminpassword&username=admin 2023-10-19 18:11:15,312 ERROR [root] (main:) Actual payload:username=admin&password=adminpassword [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.423 s <<< FAILURE! - in com.cloud.network.nicira.NiciraRestClientTest [ERROR] testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect(com.cloud.network.nicira.NiciraRestClientTest) Time elapsed: 1.413 s <<< ERROR! java.lang.NullPointerException ``` Passing case(order not shuffled): ``` 2023-10-19 18:11:09,513 ERROR [root] (main:) Wanted payload:username=admin&password=adminpassword 2023-10-19 18:11:09,517 ERROR [root] (main:) Actual payload:username=admin&password=adminpassword [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.968 s - in com.cloud.network.nicira.NiciraRestClientTest ``` The test class configures login parameters here: https://github.com/apache/cloudstack/blob/fcbf540369d5d51145573dc948e21930531a3fad/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraRestClientTest.java#L82-L93 and the source code builds them here: https://github.com/apache/cloudstack/blob/fcbf540369d5d51145573dc948e21930531a3fad/plugins/network-elements/nicira-nvp/src/main/java/com/cloud/network/nicira/NiciraRestClient.java#L122-L131 ### Fix To make the map ordered for ensuring deterministic results during serialization/deserialization, changes were done at the source code and test by converting the `HashMap` to a `LinkedHashMap`. ### Additional fixes The change also fixed following tests in the same class which were flaky due to the same reason: - com.cloud.network.nicira.NiciraRestClientTest#testExecuteTwoConsecutiveUnauthorizedExecutions - com.cloud.network.nicira.NiciraRestClientTest#testExecuteUnauthorizedThenSuccess ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] build/CI ### Feature/Enhancement Scale or Bug Severity The fix is backward compatible as it merely changes an unordered map implementation to an ordered one for ensuring that when string serialization of the map is done, results are deterministic. #### Feature/Enhancement Scale - [ ] Major - [x] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [x] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? - The test passes when run with the NonDex tool(`mvn -pl plugins/network-elements/nicira-nvp edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=com.cloud.network.nicira.NiciraRestClientTest#testExecuteLiveLockWhenControllerAllowsLoginAndFollowsWithUnauthorizedButDoesNotRediect`). - The entire test suite of the module passes(`mvn -pl plugins/network-elements/nicira-nvp test`). #### How did you try to break this feature and the system with this change? The change does not affect any feature as it only makes an unordered data structured as ordered. This does not enforce any constraints on source code and is an internal implementation detail. However it makes the code more deterministic in cases where serialization to strings is done directly, which is the case for some tests <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
