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

ConfigurationLoaderTest::testIncorrectTag is flaky #4664

Closed
flakycov opened this Issue Jul 8, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@flakycov

flakycov commented Jul 8, 2017

from TestingResearchIllinois/NonDex#112

ConfigurationLoaderTest.testIncorrectTag failed for us on commit 82bc23f but did not fail on the corresponding Travis run.

Our tool confirms this test is flaky. The full stacktrace is below. We set up our build environment to be as close as possible to your Travis build environment, using Xcode 7.3.1, OS X 10.11, Java 1.8.0_92 and Maven 3.3.9.

It seems Nondex should also have caught this flaky test. @alexgyori, @romani ?

Tests run: 25, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.299 sec <<< FAILURE!
testIncorrectTag(com.puppycrawl.tools.checkstyle.ConfigurationLoaderTest)  Time elapsed: 0.015 sec  <<< ERROR!
java.lang.IllegalArgumentException: argument type mismatch
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at com.puppycrawl.tools.checkstyle.ConfigurationLoaderTest.testIncorrectTag(ConfigurationLoaderTest.java:384)

This is the line in question:

final Object obj = constructor.newInstance(objParent);

@flakycov flakycov changed the title from ConfigurationLoaderTest.testIncorrectTag is flaky to ConfigurationLoaderTest::testIncorrectTag is flaky Jul 8, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

@flakycov Do you have this problem in the current master commit? Do you need this specific commit for some reason?

argument type mismatch

Can you clarify what the argument was that was sent versus what it is expecting?
InternalLoader only has 1 constructor, so how can it be choosing the wrong one?

It seems Nondex should also have caught this flaky test.

Do you mean because of the result of getDeclaredConstructors? If you can confirm an issue, you should report it to them. We can't do anything about it.

Member

rnveach commented Jul 8, 2017

@flakycov Do you have this problem in the current master commit? Do you need this specific commit for some reason?

argument type mismatch

Can you clarify what the argument was that was sent versus what it is expecting?
InternalLoader only has 1 constructor, so how can it be choosing the wrong one?

It seems Nondex should also have caught this flaky test.

Do you mean because of the result of getDeclaredConstructors? If you can confirm an issue, you should report it to them. We can't do anything about it.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

InternalLoader only has 1 constructor, so how can it be choosing the wrong one?

I actually debugged this in the latest master and there is in-fact 2.
One is: public com.puppycrawl.tools.checkstyle.ConfigurationLoader$InternalLoader(org.powermock.core.IndicateReloadClass)
The second is: public com.puppycrawl.tools.checkstyle.ConfigurationLoader$InternalLoader(com.puppycrawl.tools.checkstyle.ConfigurationLoader) throws org.xml.sax.SAXException,javax.xml.parsers.ParserConfigurationException

It looks like powermock is interfering with the reflection even though the specific test doesn't use powermock.
I can confirm without powermock there is only 1 constructor in the exact same test.

@romani This looks like a valid issue to me even if we aren't seeing any negative results right now.
For this test, we should specify exactly which constructor we want and not loop through them all.

Member

rnveach commented Jul 8, 2017

InternalLoader only has 1 constructor, so how can it be choosing the wrong one?

I actually debugged this in the latest master and there is in-fact 2.
One is: public com.puppycrawl.tools.checkstyle.ConfigurationLoader$InternalLoader(org.powermock.core.IndicateReloadClass)
The second is: public com.puppycrawl.tools.checkstyle.ConfigurationLoader$InternalLoader(com.puppycrawl.tools.checkstyle.ConfigurationLoader) throws org.xml.sax.SAXException,javax.xml.parsers.ParserConfigurationException

It looks like powermock is interfering with the reflection even though the specific test doesn't use powermock.
I can confirm without powermock there is only 1 constructor in the exact same test.

@romani This looks like a valid issue to me even if we aren't seeing any negative results right now.
For this test, we should specify exactly which constructor we want and not loop through them all.

@rnveach rnveach added the approved label Jul 8, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

It seems Nondex should also have caught this flaky test.

@flakycov It did. We suppressed it during TestingResearchIllinois/NonDex#112 .
This is why nondex didn't seem to catch it. We assumed it was a nondex issue and not with our test.
I thought this problem seemed familiar.

If we make test specify exact constructor, we should be able to remove the suppression we added.

Member

rnveach commented Jul 8, 2017

It seems Nondex should also have caught this flaky test.

@flakycov It did. We suppressed it during TestingResearchIllinois/NonDex#112 .
This is why nondex didn't seem to catch it. We assumed it was a nondex issue and not with our test.
I thought this problem seemed familiar.

If we make test specify exact constructor, we should be able to remove the suppression we added.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

@flakycov can you confirm if this is just a personal issue or was this based on problem we filled with nondex? Why are you using this specific commit?

Member

rnveach commented Jul 8, 2017

@flakycov can you confirm if this is just a personal issue or was this based on problem we filled with nondex? Why are you using this specific commit?

@alexgyori

This comment has been minimized.

Show comment
Hide comment
@alexgyori

alexgyori Jul 8, 2017

@rnveach you are correct; there was a related issue in TestingResearchIllinois/NonDex#112

@flakycov ran into it through some other means, and then I noticed that you already pointed to something related after reasoning that NonDex should catch this.

After debugging more, it seems that indeed, as @rnveach mentions, powermock adds one extra constructor at runtime, and depending on whether it is inserted as the last one or the first one, the test may break (and that's what @flakycov observed). NonDex would just proactively explore different orders and flag the code; that's what you observed in the issue you filed.

I think the test code should be changed and instead of those loops just get the correct constructor. Do you want me to make a PR for that?

alexgyori commented Jul 8, 2017

@rnveach you are correct; there was a related issue in TestingResearchIllinois/NonDex#112

@flakycov ran into it through some other means, and then I noticed that you already pointed to something related after reasoning that NonDex should catch this.

After debugging more, it seems that indeed, as @rnveach mentions, powermock adds one extra constructor at runtime, and depending on whether it is inserted as the last one or the first one, the test may break (and that's what @flakycov observed). NonDex would just proactively explore different orders and flag the code; that's what you observed in the issue you filed.

I think the test code should be changed and instead of those loops just get the correct constructor. Do you want me to make a PR for that?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 8, 2017

Member

I think the test code should be changed and instead of those loops just get the correct constructor. Do you want me to make a PR for that?

The PR has already been started at #4666 , we are just awaiting the results now to see if it passes without the exclusion.

Member

rnveach commented Jul 8, 2017

I think the test code should be changed and instead of those loops just get the correct constructor. Do you want me to make a PR for that?

The PR has already been started at #4666 , we are just awaiting the results now to see if it passes without the exclusion.

@romani romani added the miscellaneous label Jul 8, 2017

romani added a commit that referenced this issue Jul 8, 2017

@romani romani added this to the 8.1 milestone Jul 8, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 8, 2017

Member

fix is merged.
@flakycov , thanks a lot !

Member

romani commented Jul 8, 2017

fix is merged.
@flakycov , thanks a lot !

@romani romani closed this Jul 8, 2017

@alexgyori

This comment has been minimized.

Show comment
Hide comment
@alexgyori

alexgyori Jul 8, 2017

Do you want to remove --fail-never or replace it with --fail-at-end?

Do you want to remove --fail-never or replace it with --fail-at-end?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 11, 2017

Member

I use fail-never to print some details to CI log before failing the build process to give contributor some details to output.
Ones you fix TestingResearchIllinois/NonDex#97 , we can remove that option from maven and simplify CI command line.

Member

romani commented Jul 11, 2017

I use fail-never to print some details to CI log before failing the build process to give contributor some details to output.
Ones you fix TestingResearchIllinois/NonDex#97 , we can remove that option from maven and simplify CI command line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment