-
Notifications
You must be signed in to change notification settings - Fork 116
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
Unit test for no resources in the target folder #1478
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the testcase I'll hopefully find some time the next days to further analyses this failure, in the meantime I think the test can even be shrinked a bit see above comments.
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
Thanks @laeubi! I have pushed adjustments you've proposed. |
Test Results 324 files +3 324 suites +3 30m 37s ⏱️ + 4m 29s For more details on these failures and errors, see this check. Results for commit 17b2d4e. ± Comparison against base commit 6e92f59. ♻️ This comment has been updated with latest results. |
pomXml.setContents(new ByteArrayInputStream("Nothing".getBytes()), true, false, null); | ||
|
||
waitForJobsToComplete(monitor); | ||
Thread.sleep(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell can you please advice here? It seems waitForJobsToComplete
is not enough here or @BoykoAlex maybe can explain why additional wait is required here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically waitForJobsToComplete
wasn't enough. Removing Thread.sleep(...)
calls makes it fail at line 76 AFAIR. There are ways to execute stuff async bypassing the Eclipse job queue, i.e. Display.asyncExec(...)
or less likely CompletableFuture
... I admit I didn't experiment any further with "waiting".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BoykoAlex can you probably replace this then with a loop that check if the file is deleted and if not waits a little time up to 5 seconds?
Any news on this? Anything we can do to provide additional help? |
I have a rough idea how to solve this but not yet finished/implemented that, |
Sounds great. Let us know if we can provide any additional help here. |
And many many many thanks for looking into this, very much appreciated !!!!!!! |
One possibility is of course to help with the upstream issue: this would already help for the case of copy resources. |
@BoykoAlex this looks good now, can you shash / rebase the testcase? |
I have squashed and rebased the commits |
Thanks, sadly even with my recent changes it still fails :-\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have some message added to the asserts so we don't need to check linenumbers in case of failure to see whats going on.
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.core.tests/src/org/eclipse/m2e/core/AppPropsNotCopiedIntoTargetTest.java
Outdated
Show resolved
Hide resolved
Added the failure messages in asserts. Squashed the commits into a single commit again. |
1771957
to
030cd07
Compare
@laeubi any idea why CI is requesting a version bump for an untouched bundle? |
This sometimes happens because of changed ECJ version. |
Indeed: https://ci.eclipse.org/m2e/job/m2e/job/master/ ; looks like build has been broken for 4 weeks already. |
It's because the license content changed
adding |
Ignoring MANIFEST fields in the comparison is IMO a last resort solution. |
030cd07
to
5ef5814
Compare
|
||
pomXml.setContents(new ByteArrayInputStream(content.getBytes()), true, false, null); | ||
waitForJobsToComplete(monitor); | ||
Thread.sleep(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here replace with a loop that checks regualry with an upper bound instead of a fixed delay.
8e84ca8
to
d5d4001
Compare
d5d4001
to
ecec91f
Compare
bf4644c
to
ab134c1
Compare
ab134c1
to
6ba865a
Compare
6ba865a
to
17b2d4e
Compare
In support of issue #1302