You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/12/13 09:10:57 UTC
[GitHub] [struts] Yilinwang opened a new pull request #458: sort results of getAnnotations() for deterministic iteration order
Yilinwang opened a new pull request #458:
URL: https://github.com/apache/struts/pull/458
The tests `com.opensymphony.xwork2.validator.AnnotationActionValidatorManagerTest.testGetValidatorsForInterface` and `com.opensymphony.xwork2.validator.AnnotationActionValidatorManagerTest.testSkipUserMarkerActionLevelShortCircuit` can fail due to a different iteration order of `Object.getAnnotations()`. The failures are as follows:
```
[ERROR] AnnotationActionValidatorManagerTest.testGetValidatorsForInterface:157
[ERROR] FreemarkerResultMockedTest.testDynamicAttributesSupport:130 expected:<... value="" id="test" [foo="bar" placeholder="input]"/><input type="text...> but was:<... value="" id="test" [placeholder="input" foo="bar]"/><input type="text...>
[ERROR] com.opensymphony.xwork2.validator.AnnotationActionValidatorManagerTest.testSkipUserMarkerActionLevelShortCircuit Time elapsed: 0.033 s <<< FAILURE!
junit.framework.AssertionFailedError: expected:<1> but was:<2>
at com.opensymphony.xwork2.validator.AnnotationActionValidatorManagerTest.testSkipUserMarkerActionLevelShortCircuit(AnnotationActionValidatorManagerTest.java:307)
```
The fix is to sort the result of `getAnnotations()` so that the iteration order remains stable and the failure will not occur any more. In this way, the test will be more stable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #458:
URL: https://github.com/apache/struts/pull/458#issuecomment-748813558
I'm against such changes which are introduced just to satisfy some external dependencies or to pass tests - this isn't a user requirement nor an architectural change. In this case we depend on JVM and if in the future the order will change (by changes in JVM) this code will introduce unnecessary burden in finding the reason behind failing user app.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart closed pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
lukaszlenart closed pull request #458:
URL: https://github.com/apache/struts/pull/458
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] yasserzamani commented on pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #458:
URL: https://github.com/apache/struts/pull/458#issuecomment-743994518
Thanks!! Could you please write down with which command, java version, environment and maven options you ran tests?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #458:
URL: https://github.com/apache/struts/pull/458#issuecomment-743996797
This change affects `JSONValidationInterceptorTest.testValidationFails` which started to fail, that means this PR is wrong.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #458:
URL: https://github.com/apache/struts/pull/458#issuecomment-748524387
Hi @Yilinwang , @yasserzamani and @lukaszlenart .
Interesting topic. 😃
It looks like this PR and PR #457 have a commonality in that they both involve instrumentation of tests using the NonDex tool. The tests pass successfully during a normal run, but some intermittent failures can appear if using NonDex (which instruments the code to change ordering behaviour, as I understand it).
Some of the existing unit tests may function under the assumption of an implicit ordering of annotations. It seems that such ordering may be stable, but not explicitly guaranteed by anything specification-wise. Others may have better information or understanding on that topic. 😅
The "implicit" order of annotation processing seems to have been in place for some time and worked "OK so far", so applying an alphabetical sort (such as in this PR) to `AnnotationValidationConfigurationBuilder` may need some careful consideration.
Applying an explicit order (alphabetical or otherwise) might provide some consistency, but could have unintended consequences. Updating tests that assume a specific order, to handle different possible ordering, could be another approach, too.
Either way it might be a good idea to clarify the expectations in the documentation (such as "the order of the annotation processing is not guaranteed", or if it is changed, "the annotations will be processed in sort order X").
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] Yilinwang commented on pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
Yilinwang commented on pull request #458:
URL: https://github.com/apache/struts/pull/458#issuecomment-744081458
Thanks for replying. The flaky tests are found by the tool [NonDex](https://github.com/TestingResearchIllinois/NonDex) using command `mvn -pl plugins/json nondex:nondex` in JDK 1.8.0_272, maven 3.6.3 and here are the stack traces when tests failed which inspired this fix.
Besides, I tested using commands `mvn -pl plugins/json test` and `mvn test` after the fix and they passed locally. Still trying to figure out what causes the Travis CI `JSONValidationInterceptorTest.testValidationFails` to fail.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #458: sort results of getAnnotations() for deterministic iteration order
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #458:
URL: https://github.com/apache/struts/pull/458#issuecomment-743996615
I don't understand how validation annotations affect order of dynamic attributes? Those are unrelated mechanisms in the code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org