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/12 05:19:19 UTC
[GitHub] [struts] Troy-Peng-97 opened a new pull request #457: Fix json validation interceptor test
Troy-Peng-97 opened a new pull request #457:
URL: https://github.com/apache/struts/pull/457
The test method testValidationFails in test class JSONValidationInterceptorTest is flaky.
The "TestUtils.normalize" function will break the order of the original list, and there are two potential orders.
["Tooshort","Thisisnoemail"], and ["Thisisnoemail", "Tooshort"]
The assertion test only test against one order: ["Tooshort","Thisisnoemail"], and thus the test will fail if the second order is returned by the normalize function.
To fix this flaky test, I add two assertion tests to verify that two strings "Tooshort" and "Thisisnoemail" do exist in the normalized string, and then check the indexes of those two strings. If the string "Thisisnoemail" has a smaller index, swap those two strings, and the order of the list is preserved.
----------------------------------------------------------------
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] coveralls commented on pull request #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #457:
URL: https://github.com/apache/struts/pull/457#issuecomment-743707063
[](https://coveralls.io/builds/35659354)
Coverage remained the same at 49.776% when pulling **198779f7b67a8d8bf40e5580ef0f32fce1efd3a2 on Troy-Peng-97:FixJSONValidationInterceptorTest** into **7840fa13335323b19ce03dc6b597b45808155282 on apache:master**.
----------------------------------------------------------------
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 a change in pull request #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on a change in pull request #457:
URL: https://github.com/apache/struts/pull/457#discussion_r546279045
##########
File path: plugins/json/src/test/java/org/apache/struts2/json/JSONValidationInterceptorTest.java
##########
@@ -66,8 +67,23 @@ public void testValidationFails() throws Exception {
String json = stringWriter.toString();
String normalizedActual = TestUtils.normalize(json, true);
-
//json
+ // fix the order of "Tooshort", and "Thisisnoemail"
Review comment:
Instead of using custom code to force the JSON string to have the order the test currently expects, I would like to suggest an alternative using the AssertJ API capabilities.
Add the following two imports:
```
import static org.assertj.core.api.Assertions.anyOf;
import org.assertj.core.api.Condition;
```
Replace the custom ordering code with this block of code (or something similar) between the //json and //execution comments:
```
//json
Condition<String> textValidationOrder1 = new Condition<>(s -> s.contains("\"text\":[\"Tooshort\",\"Thisisnoemail\"]"), "textValidationOrder1");
Condition<String> textValidationOrder2 = new Condition<>(s -> s.contains("\"text\":[\"Thisisnoemail\",\"Tooshort\"]"), "textValidationOrder2");
assertThat(normalizedActual)
.contains("\"errors\":[\"Generalerror\"]")
.contains("\"fieldErrors\":{")
.contains("\"value\":[\"Minvalueis-1\"]")
.contains("\"password\":[\"Passwordisn'tcorrect\"]");
assertThat(normalizedActual).is(anyOf(textValidationOrder1, textValidationOrder2));
//execution
```
Others might have different suggestions, or alternative ways to use the AssertJ API to handle the possibility of the order changing for the validation annotations.
##########
File path: plugins/json/src/test/java/org/apache/struts2/json/JSONValidationInterceptorTest.java
##########
@@ -33,6 +33,7 @@
import org.apache.struts2.interceptor.validation.AnnotationValidationInterceptor;
import org.apache.struts2.interceptor.validation.SkipValidation;
import org.apache.struts2.util.TestUtils;
+import org.assertj.core.internal.bytebuddy.asm.Advice;
Review comment:
Appears to be an unused import (possibly used in an older revision ?)
----------------------------------------------------------------
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 #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
lukaszlenart closed pull request #457:
URL: https://github.com/apache/struts/pull/457
----------------------------------------------------------------
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
Unsubscribe me from the email list
Posted by Murugasen Kalimuthu <cb...@gmail.com>.
On Mon, Dec 14, 2020 at 5:28 AM GitBox <gi...@apache.org> wrote:
>
> Troy-Peng-97 edited a comment on pull request #457:
> URL: https://github.com/apache/struts/pull/457#issuecomment-744081757
>
>
> Yeah, I think the setText() function might be the reason for this flaky
> test.
> > Could you indicate what Java JDK Type / JDK version / OS combination
> you were using when you saw the test failure ?
>
> I am using Java 1.8.0_275 / Mac OS 10.14.6, and the failure rate is
> pretty high (30% or 40%)
> Specifically, Running Maven test multiple times will detect the failure
> easily. I am using UIUC Nondex tools to find and debug this flaky
> test.(Checkout https://github.com/TestingResearchIllinois/NonDex)
>
>
> ----------------------------------------------------------------
> 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] Troy-Peng-97 edited a comment on pull request #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
Troy-Peng-97 edited a comment on pull request #457:
URL: https://github.com/apache/struts/pull/457#issuecomment-744081757
Yeah, I think the setText() function might be the reason for this flaky test.
> Could you indicate what Java JDK Type / JDK version / OS combination you were using when you saw the test failure ?
I am using Java 1.8.0_275 / Mac OS 10.14.6, and the failure rate is pretty high (30% or 40%)
Specifically, Running Maven test multiple times will detect the failure easily. I am using UIUC Nondex tools to find and debug this flaky test.(Checkout https://github.com/TestingResearchIllinois/NonDex)
----------------------------------------------------------------
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] yasserzamani commented on pull request #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #457:
URL: https://github.com/apache/struts/pull/457#issuecomment-743751541
Weird! I see it should only replace spaces \r \n tab. Anyway I think the fix could be inside the normalize method itself?
----------------------------------------------------------------
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 #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #457:
URL: https://github.com/apache/struts/pull/457#issuecomment-748517389
Hi @Troy-Peng-97 . Thanks for replying with the additional information. 😃
From your last reply, it looks like the test `testValidationFails()` is only actually failing when you are running it using the NonDex tool, correct ?
I tried out NonDex briefly (_thanks for providing the reference link to the project_), and if I understand correctly, that tool creates instrumented versions of classes and uses those to change the order-of-execution.
So the `testValidationFails()` is only failing when NonDex instruments it, and not when running the standard build for Struts2 Core, correct ?
Assuming the standard build does not appear to fail after repeated-runs (only failing when running NonDex instrumentation), whether the test is really flaky or not may depend on how the ordering of the annotations is handled by a particular JVM.
It seems that the processing ordering must be pretty stable, otherwise we would expect the test to fail randomly, even without NonDex instrumentation involved. Internet searches did not uncover an absolute answer concerning annotation ordering (in terms of execution or values returned by `getAnnotations()`). There may be an implicit order that works most of the time, but it seems it may not be explicitly guaranteed by any specification.
Given what you reported, and the discussion so far, updating this particular test to not depend on a particular ordering of the annotation result probably makes some sense. If anyone thinks otherwise, they can add a comment ?
----------------------------------------------------------------
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 #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #457:
URL: https://github.com/apache/struts/pull/457#issuecomment-743788703
Hi @Troy-Peng-97 .
Were you able to cause the `testValidationFails()` to fail during a unit test execution ? If so, how frequently can you cause the failure to happen ? (I tried re-running the test several times with no failures).
Could you indicate what Java JDK Type / JDK version / OS combination you were using when you saw the test failure ?
The `TestUtils.normalize()` method does not appear to capable of changing string content order, unless `Matcher` is doing something weird. As @yasserzamani mentioned, only "whitespace" characters should be impacted.
If the test is occasionally failing, it could mean the _validator annotations_ for the `setText()` are sometimes being executed in a different order than they are declared (or ending up with a result "as if" they were executed in a different order).
It definitely seems weird, given the conditions. If `testValidationFails()` is failing randomly, it could point to an intermittent issue somewhere else. If possible, that should be confirmed before changing the unit test, just to make sure it is the unit test that has the problem (and not code elsewhere). :smile:
----------------------------------------------------------------
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] Troy-Peng-97 commented on pull request #457: Fix json validation interceptor test
Posted by GitBox <gi...@apache.org>.
Troy-Peng-97 commented on pull request #457:
URL: https://github.com/apache/struts/pull/457#issuecomment-744081757
Yeah, I think the setText() function might be the reason for this flaky test.
> Could you indicate what Java JDK Type / JDK version / OS combination you were using when you saw the test failure ?
I am using Java 1.8.0_275 / Mac OS 10.14.6, and the failure rate is pretty high (30% or 40%)
Specifically, Running Maven test multiple times will detect the failure easily. I am using UIUC Nondex tools to find and debug this flaky test.(Checkout https://github.com/TestingResearchIllinois/NonDex)
----------------------------------------------------------------
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