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

Checkstyle tests should not require internet #3536

Closed
rnveach opened this Issue Nov 9, 2016 · 16 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Nov 9, 2016

Since moving the import DTD to sourceforge, we have been getting intermittent 403 errors on PRs, master builds, and locally (for me personally).
Example:
https://travis-ci.org/checkstyle/checkstyle/jobs/174479836#L497

checkstyle.checks.imports.ImportControlCheckTest.testTwoRegExp(ImportControlCheckTest.java:177)
Caused by: java.lang.reflect.InvocationTargetException
	at com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest.testTwoRegExp(ImportControlCheckTest.java:177)
Caused by: org.apache.commons.beanutils.ConversionException: Unable to load /home/travis/build/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/imports/import-control_two-re.xml
	at com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest.testTwoRegExp(ImportControlCheckTest.java:177)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: unable to read file:/home/travis/build/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/imports/import-control_two-re.xml
	at com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest.testTwoRegExp(ImportControlCheckTest.java:177)
Caused by: java.io.IOException: Server returned HTTP response code: 403 for URL: http://checkstyle.sourceforge.net/dtds/import_control_1_2.dtd
	at com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheckTest.testTwoRegExp(ImportControlCheckTest.java:177)

and
[ERROR] around Ant part ...<ant antfile="config/ant-phase-verify.xml"/>... @ 7:47 in /home/travis/build/checkstyle/checkstyle/target/antrun/build-main.xml: cannot initialize module TreeWalker - Cannot set property 'file' to '/home/travis/build/checkstyle/checkstyle/config/import-control.xml' in module ImportControl: InvocationTargetException: Unable to load /home/travis/build/checkstyle/checkstyle/config/import-control.xml: unable to read file:/home/travis/build/checkstyle/checkstyle/config/import-control.xml: Server returned HTTP response code: 403 for URL: http://checkstyle.sourceforge.net/dtds/import_control_1_2.dtd

We should either create our own proxy on tests and override connections to the internet (to simulate being online) and have an offline mode for DTD validation (or skip it entirely).
The second will require changes to the CLI and the Ant task.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 10, 2016

Member

Strange, we had such problem on moment on when this dtd is introduced, after some time it is cached somehow and did not see such provlems for a while on ci and local.
Please look at pr where dtd was intriduced, it has shell script that test connection.

script is there - #3438 (comment)

Member

romani commented Nov 10, 2016

Strange, we had such problem on moment on when this dtd is introduced, after some time it is cached somehow and did not see such provlems for a while on ci and local.
Please look at pr where dtd was intriduced, it has shell script that test connection.

script is there - #3438 (comment)

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 10, 2016

Member

Local failure for me is when I am at work behind a firewall. It is windows only so I can't run shells.

@romani
Were we suppose to update the ImportControlLoader for the new DTD? 1.2 isn't listed.


DTD_RESOURCE_BY_ID.put(DTD_PUBLIC_ID_1_1, DTD_RESOURCE_NAME_1_1);

When testing proxy, I am only seeing connections for invalid URLs and for import_control_1_2.dtd.

Member

rnveach commented Nov 10, 2016

Local failure for me is when I am at work behind a firewall. It is windows only so I can't run shells.

@romani
Were we suppose to update the ImportControlLoader for the new DTD? 1.2 isn't listed.


DTD_RESOURCE_BY_ID.put(DTD_PUBLIC_ID_1_1, DTD_RESOURCE_NAME_1_1);

When testing proxy, I am only seeing connections for invalid URLs and for import_control_1_2.dtd.

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 10, 2016

@romani romani added the approved label Nov 10, 2016

romani added a commit that referenced this issue Nov 10, 2016

@romani romani added the bug label Nov 10, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 10, 2016

Member

Were we suppose to update the ImportControlLoader for the new DTD? 1.2 isn't listed.

yes, missed update during #2999.
PR is merged.

When testing proxy, I am only seeing connections for invalid URLs and for import_control_1_2.dtd.

I do not understand this, please state the re-phrase.

Member

romani commented Nov 10, 2016

Were we suppose to update the ImportControlLoader for the new DTD? 1.2 isn't listed.

yes, missed update during #2999.
PR is merged.

When testing proxy, I am only seeing connections for invalid URLs and for import_control_1_2.dtd.

I do not understand this, please state the re-phrase.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 10, 2016

Member

When testing proxy, I am only seeing connections for invalid URLs and for import_control_1_2.dtd.
I do not understand this, please state the re-phrase.

Issue was to make tests completely offline. I inserted a proxy into the tests to prevent connections to the internet and had it print what requests the tests were making.

One was import_control_1_2.dtd which lead me to the fix I made.
Others were for files that don't exist and are part of the test:

http://checkstyle.sourceforge.net/non_existing_suppression.xml
http://checkstyle.sourceforge.net/files/suppressions_none.xml
http://mock.sourceforge.net/suppressions_none.xml
UnableToLoadThisURL:443
Member

rnveach commented Nov 10, 2016

When testing proxy, I am only seeing connections for invalid URLs and for import_control_1_2.dtd.
I do not understand this, please state the re-phrase.

Issue was to make tests completely offline. I inserted a proxy into the tests to prevent connections to the internet and had it print what requests the tests were making.

One was import_control_1_2.dtd which lead me to the fix I made.
Others were for files that don't exist and are part of the test:

http://checkstyle.sourceforge.net/non_existing_suppression.xml
http://checkstyle.sourceforge.net/files/suppressions_none.xml
http://mock.sourceforge.net/suppressions_none.xml
UnableToLoadThisURL:443
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 10, 2016

Member

for 1st an 2nd - I remember.
not sure why there 3rd and 4th, but it might be some test for bad url.

do you still have a problem ?

Member

romani commented Nov 10, 2016

for 1st an 2nd - I remember.
not sure why there 3rd and 4th, but it might be some test for bad url.

do you still have a problem ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 15, 2016

Member

With fix I don't have as big as problem as before.
Some tests that require internet still fail for me like SuppressionFilterTest.testRemoteFileExternalResourceContentDoesNotChange.

Member

rnveach commented Nov 15, 2016

With fix I don't have as big as problem as before.
Some tests that require internet still fail for me like SuppressionFilterTest.testRemoteFileExternalResourceContentDoesNotChange.

@romani

This comment has been minimized.

Show comment
Hide comment
Member

romani commented Nov 16, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 16, 2016

Member

http://stackoverflow.com/a/1689309/1015848

We already use assume on this test, but junit and maven still puts it as an error.

Tests run: 11, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 65.615 sec <<< FAILURE! - in com.puppycrawl.tools.checkstyle.filters.SuppressionFilterTest
testRemoteFileExternalResourceContentDoesNotChange(com.puppycrawl.tools.checkstyle.filters.SuppressionFilterTest)  Time elapsed: 42.168 sec  <<< ERROR!
org.junit.AssumptionViolatedException: got: <false>, expected: is <true>
    at com.puppycrawl.tools.checkstyle.filters.SuppressionFilterTest.testRemoteFileExternalResourceContentDoesNotChange(SuppressionFilterTest.java:232)

I believe this is because PowerMock is interfering with the Assumption exception and junit doesn't see it as an assumption failure.

Conditionals may work, but then couldn't it hide a bad test especially in future changes with CI?

If we switched to using a proxy, we wouldn't have to worry about fluctuating internet connections.

Member

rnveach commented Nov 16, 2016

http://stackoverflow.com/a/1689309/1015848

We already use assume on this test, but junit and maven still puts it as an error.

Tests run: 11, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 65.615 sec <<< FAILURE! - in com.puppycrawl.tools.checkstyle.filters.SuppressionFilterTest
testRemoteFileExternalResourceContentDoesNotChange(com.puppycrawl.tools.checkstyle.filters.SuppressionFilterTest)  Time elapsed: 42.168 sec  <<< ERROR!
org.junit.AssumptionViolatedException: got: <false>, expected: is <true>
    at com.puppycrawl.tools.checkstyle.filters.SuppressionFilterTest.testRemoteFileExternalResourceContentDoesNotChange(SuppressionFilterTest.java:232)

I believe this is because PowerMock is interfering with the Assumption exception and junit doesn't see it as an assumption failure.

Conditionals may work, but then couldn't it hide a bad test especially in future changes with CI?

If we switched to using a proxy, we wouldn't have to worry about fluctuating internet connections.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 16, 2016

Member

http://junit.sourceforge.net/javadoc/org/junit/Assume.html

The default JUnit runner treats tests with failing assumptions as ignored. Custom runners may behave differently.

Looks like smth(powermock) change default Runner.

Can you try Rule instead ?

Proxy is overcomplication for such simple issue.

Member

romani commented Nov 16, 2016

http://junit.sourceforge.net/javadoc/org/junit/Assume.html

The default JUnit runner treats tests with failing assumptions as ignored. Custom runners may behave differently.

Looks like smth(powermock) change default Runner.

Can you try Rule instead ?

Proxy is overcomplication for such simple issue.

linelect added a commit to linelect/checkstyle that referenced this issue Nov 17, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 17, 2016

Member

@romani It doesn't work. It too uses an assumption.

org.junit.AssumptionViolatedException: Ignored by MyTest
    at org.junit.Assume.assumeTrue(Assume.java:59)
    at com.puppycrawl.tools.checkstyle.internal.ConditionalIgnoreRule$IgnoreStatement.evaluate(ConditionalIgnoreRule.java:110)
    at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.evaluateStatement(PowerMockJUnit47RunnerDelegateImpl.java:107)
Member

rnveach commented Nov 17, 2016

@romani It doesn't work. It too uses an assumption.

org.junit.AssumptionViolatedException: Ignored by MyTest
    at org.junit.Assume.assumeTrue(Assume.java:59)
    at com.puppycrawl.tools.checkstyle.internal.ConditionalIgnoreRule$IgnoreStatement.evaluate(ConditionalIgnoreRule.java:110)
    at org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.evaluateStatement(PowerMockJUnit47RunnerDelegateImpl.java:107)
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 17, 2016

Member

ok , looks like simple IF condition is the most robust solution, please do, please add comment above to explain why this is not Assume and Rule :), it is not so obvious.

Member

romani commented Nov 17, 2016

ok , looks like simple IF condition is the most robust solution, please do, please add comment above to explain why this is not Assume and Rule :), it is not so obvious.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 19, 2016

Member

@rnveach , please confirm that you do not have any issues with this anymore (only you can verify this issue). Please close if it is actually fixed.

Member

romani commented Nov 19, 2016

@rnveach , please confirm that you do not have any issues with this anymore (only you can verify this issue). Please close if it is actually fixed.

@romani romani added this to the 7.3 milestone Nov 19, 2016

@romani romani added miscellaneous and removed bug labels Nov 19, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 19, 2016

Member

@romani

only you can verify this issue

This is not true, anyone can test it.
Since test requires internet, all you have to do is go off offline (either unplug ethernet or disable wifi) and issue will present itself.

Member

rnveach commented Nov 19, 2016

@romani

only you can verify this issue

This is not true, anyone can test it.
Since test requires internet, all you have to do is go off offline (either unplug ethernet or disable wifi) and issue will present itself.

@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Mar 2, 2018

Collaborator

The site checkstyle.sourceforge.net is still on maintenance. And suppression tests are still failing.
Maybe we should move the test content to github.io?
Or make a mirror and use github.io as back up for sourceforge.net in the test?

Collaborator

pbludov commented Mar 2, 2018

The site checkstyle.sourceforge.net is still on maintenance. And suppression tests are still failing.
Maybe we should move the test content to github.io?
Or make a mirror and use github.io as back up for sourceforge.net in the test?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 7, 2018

Member

Originally tests were hitting github.com (not a github.io) , so they were even more unstable as file got error to download even there were no maintenance.

Or make a mirror and use github.io as back up for sourceforge.net in the test?

Probably yes, it might be, beneficial to have copy of web site at GitHub.io , or maybe be eventually migrate to github.io completely.

Member

romani commented Mar 7, 2018

Originally tests were hitting github.com (not a github.io) , so they were even more unstable as file got error to download even there were no maintenance.

Or make a mirror and use github.io as back up for sourceforge.net in the test?

Probably yes, it might be, beneficial to have copy of web site at GitHub.io , or maybe be eventually migrate to github.io completely.

@rnveach

This comment has been minimized.

Show comment
Hide comment
Member

rnveach commented May 3, 2018

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