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

MainTest.testCreateListenerWithLocationIllegalStateException isn't deleting it's test file anymore #4842

Closed
rnveach opened this Issue Jul 28, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@rnveach
Member

rnveach commented Jul 28, 2017

Issue created and identified after #4832 (comment) was merged,

Something happened in this PR to the test testCreateListenerWithLocationIllegalStateException in MainTest.
Before this commit, the file myfolder123 was created and deleted by this test.
After this commit, this file is no longer deleted and because it is placed inside the repository, I am getting a checkstyle violation on it that it doesn't end with a new line.

I am not getting any exceptions or anything to understand why it isn't being deleted anymore, it is just returning false.

This should be fixed so it doesn't cause issues during multiple test and verification runs.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 29, 2017

Member

@rnveach , I can not reproduce it, please provide prove from command line.
Do you have problem on windows ?

My commands:

$ mvn verify
....
$ mvn verify
...
$ mvn --version
Apache Maven 3.3.9 (bb52d8502b132ec0a5a3f4c09453c07478323dc5; 2015-11-10T08:41:47-08:00)
Maven home: /opt/maven/apache-maven-3.3.9
Java version: 1.8.0_131, vendor: Oracle Corporation
Java home: /opt/jvm/jdk1.8.0_131/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.4.0-87-generic", arch: "amd64", family: "unix"
Member

romani commented Jul 29, 2017

@rnveach , I can not reproduce it, please provide prove from command line.
Do you have problem on windows ?

My commands:

$ mvn verify
....
$ mvn verify
...
$ mvn --version
Apache Maven 3.3.9 (bb52d8502b132ec0a5a3f4c09453c07478323dc5; 2015-11-10T08:41:47-08:00)
Maven home: /opt/maven/apache-maven-3.3.9
Java version: 1.8.0_131, vendor: Oracle Corporation
Java home: /opt/jvm/jdk1.8.0_131/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.4.0-87-generic", arch: "amd64", family: "unix"
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 29, 2017

Member

@romani The issue showed itself on 2 different windows machines, but not on a linux VM.

Here is the output of mvn clean verify: https://justpaste.it/19he5
Please note, I don't need to run it twice to see problems because the tests run before the checkstyle. After the tests runs, checkstyle finds the file that isn't being deleted.

Here is the output of mvn --version:

Apache Maven 3.2.1 (ea8b2b07643dbb1b84b6d16e1f08391b666bc1e9; 2014-02-14T12:37:52-05:00)
Maven home: D:\Program Files\apache-maven-3.2.1
Java version: 1.8.0_77, vendor: Oracle Corporation
Java home: D:\Program Files\JDK8\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 7", version: "6.1", arch: "x86", family: "dos"

I can also reproduce the problem running the junit in Eclipse IDE, so it is not connected to maven.

Member

rnveach commented Jul 29, 2017

@romani The issue showed itself on 2 different windows machines, but not on a linux VM.

Here is the output of mvn clean verify: https://justpaste.it/19he5
Please note, I don't need to run it twice to see problems because the tests run before the checkstyle. After the tests runs, checkstyle finds the file that isn't being deleted.

Here is the output of mvn --version:

Apache Maven 3.2.1 (ea8b2b07643dbb1b84b6d16e1f08391b666bc1e9; 2014-02-14T12:37:52-05:00)
Maven home: D:\Program Files\apache-maven-3.2.1
Java version: 1.8.0_77, vendor: Oracle Corporation
Java home: D:\Program Files\JDK8\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 7", version: "6.1", arch: "x86", family: "dos"

I can also reproduce the problem running the junit in Eclipse IDE, so it is not connected to maven.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 29, 2017

Member

@Nimfadora , do you have windows pc to test this ?

strange that appveyor does not show us this problem.

Member

romani commented Jul 29, 2017

@Nimfadora , do you have windows pc to test this ?

strange that appveyor does not show us this problem.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 29, 2017

Contributor

@romani, @rnveach yes I have windows environment, will try to reproduce it

Contributor

Nimfadora commented Jul 29, 2017

@romani, @rnveach yes I have windows environment, will try to reproduce it

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 29, 2017

Member

strange that appveyor does not show us this problem.

@romani appveyor doesn't run a full verify, it is split into separate chunks.
1 run is checkstyle: https://github.com/checkstyle/checkstyle/blob/master/appveyor.yml#L56
A completely separate run is verify without checkstyle: https://github.com/checkstyle/checkstyle/blob/master/appveyor.yml#L60

travis looks like it is setup the same way.

Member

rnveach commented Jul 29, 2017

strange that appveyor does not show us this problem.

@romani appveyor doesn't run a full verify, it is split into separate chunks.
1 run is checkstyle: https://github.com/checkstyle/checkstyle/blob/master/appveyor.yml#L56
A completely separate run is verify without checkstyle: https://github.com/checkstyle/checkstyle/blob/master/appveyor.yml#L60

travis looks like it is setup the same way.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 30, 2017

Member

@timurt @romani Modified appveyor reproduced the problem. Travis did not.
#4852
https://ci.appveyor.com/project/Checkstyle/checkstyle/build/8947#L3972

[echo] Checkstyle started (checkstyle_non_main_files_checks.xml): 30/07/2017 03:19:14 AM
[checkstyle] Running Checkstyle 8.2-SNAPSHOT on 1283 files
[checkstyle] [ERROR] C:\projects\checkstyle\myfolder123:0: File does not end with a newline. [NewlineAtEndOfFile]

https://travis-ci.org/checkstyle/checkstyle/builds/259004817

Member

rnveach commented Jul 30, 2017

@timurt @romani Modified appveyor reproduced the problem. Travis did not.
#4852
https://ci.appveyor.com/project/Checkstyle/checkstyle/build/8947#L3972

[echo] Checkstyle started (checkstyle_non_main_files_checks.xml): 30/07/2017 03:19:14 AM
[checkstyle] Running Checkstyle 8.2-SNAPSHOT on 1283 files
[checkstyle] [ERROR] C:\projects\checkstyle\myfolder123:0: File does not end with a newline. [NewlineAtEndOfFile]

https://travis-ci.org/checkstyle/checkstyle/builds/259004817

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 1, 2017

Member

appveyor doesn't run a full verify, it is split into separate chunks.

it was done for performance purposes: if file-system cache(.m2 folder) is lost (that was frequent some time ago), whole appveyor failed to finish due to timeout on execution.

Member

romani commented Aug 1, 2017

appveyor doesn't run a full verify, it is split into separate chunks.

it was done for performance purposes: if file-system cache(.m2 folder) is lost (that was frequent some time ago), whole appveyor failed to finish due to timeout on execution.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Aug 7, 2017

Contributor

I met this problem as well. Is there a way to skip this locally, except for changing suppression file?

Contributor

Luolc commented Aug 7, 2017

I met this problem as well. Is there a way to skip this locally, except for changing suppression file?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 7, 2017

Member

@Luolc , no progress on this issue as it is reproducible only on Windows, only rnveach and probably you use windows for development. Only you can resolve this issue.

Member

romani commented Aug 7, 2017

@Luolc , no progress on this issue as it is reproducible only on Windows, only rnveach and probably you use windows for development. Only you can resolve this issue.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Aug 7, 2017

Contributor

@romani I use Windows at my intern office and use MacOS at home. I checked the code and it is really weird that the file was not deleted...

Contributor

Luolc commented Aug 7, 2017

@romani I use Windows at my intern office and use MacOS at home. I checked the code and it is really weird that the file was not deleted...

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

@romani

Only you can resolve this issue.

Why was @Nimfadora assigned to this issue than if he cannot fix it? Assignments are suppose to be indications that someone else if working on the issue and shouldn't be taken.

The mocking is what is causing the failure. There is no way to rewrite the test to avoid mocking like we have seen in other tests?

Member

rnveach commented Aug 7, 2017

@romani

Only you can resolve this issue.

Why was @Nimfadora assigned to this issue than if he cannot fix it? Assignments are suppose to be indications that someone else if working on the issue and shouldn't be taken.

The mocking is what is causing the failure. There is no way to rewrite the test to avoid mocking like we have seen in other tests?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 7, 2017

Member

Why was @Nimfadora assigned to this issue than if he cannot fix it?

good point.
@Nimfadora , do you have Windows ready to test this problem ? or you potentially could have Windows PC , that mean that it is unlikely to happen soon. But other users experience this problem right now.

Member

romani commented Aug 7, 2017

Why was @Nimfadora assigned to this issue than if he cannot fix it?

good point.
@Nimfadora , do you have Windows ready to test this problem ? or you potentially could have Windows PC , that mean that it is unlikely to happen soon. But other users experience this problem right now.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Aug 7, 2017

Contributor

@romani I have windows os and if it is more critical than idea inspections I will start from fixing it today.

Contributor

Nimfadora commented Aug 7, 2017

@romani I have windows os and if it is more critical than idea inspections I will start from fixing it today.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 8, 2017

Member

please do in the middle of inspection fixes

Member

romani commented Aug 8, 2017

please do in the middle of inspection fixes

@romani romani added the GSoC2017 label Aug 11, 2017

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Aug 12, 2017

Contributor

@rnveach @romani this problem is somehow connected to mocking CommonUtils with PowerMock. I tried many variants: with temp file, System#gc(), force delete, and every attempt fails till method uses mockStatic(). When I have tried force delete it printed that file cannot be deleted. So we have two options here: remove check of CommonUtils.close() than the profile threshold should be decreased to 96. Or leave it as is, do some if (if windows don't check CommonUtils.close()).

Contributor

Nimfadora commented Aug 12, 2017

@rnveach @romani this problem is somehow connected to mocking CommonUtils with PowerMock. I tried many variants: with temp file, System#gc(), force delete, and every attempt fails till method uses mockStatic(). When I have tried force delete it printed that file cannot be deleted. So we have two options here: remove check of CommonUtils.close() than the profile threshold should be decreased to 96. Or leave it as is, do some if (if windows don't check CommonUtils.close()).

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 18, 2017

Member

@Nimfadora , we can not reduce coverage. It will cost us more.
We can not do exclusions for Windows only, as will reduce coverage too.
Ideally it would be good to open issue on powermock, but we are in big rush to finish GSoC.

My proposal as workaround:
Create folder outside of repository in OS temp folder with unique name , so it will not be deleted on Windows, but that is fine as it will not make problems. Alternative is to create temp and unique folders in target folder so eventually it will be removed.

Member

romani commented Aug 18, 2017

@Nimfadora , we can not reduce coverage. It will cost us more.
We can not do exclusions for Windows only, as will reduce coverage too.
Ideally it would be good to open issue on powermock, but we are in big rush to finish GSoC.

My proposal as workaround:
Create folder outside of repository in OS temp folder with unique name , so it will not be deleted on Windows, but that is fine as it will not make problems. Alternative is to create temp and unique folders in target folder so eventually it will be removed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 26, 2017

Member

fix is done by @liscju.
@liscju , thanks a lot !

Member

romani commented Aug 26, 2017

fix is done by @liscju.
@liscju , thanks a lot !

@romani romani closed this Aug 26, 2017

@romani romani added this to the 8.2 milestone Aug 26, 2017

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