Skip to content
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

120 P2Tests fail on master since I20231201-1800 #967

Closed
iloveeclipse opened this issue Dec 2, 2023 · 14 comments · Fixed by #984
Closed

120 P2Tests fail on master since I20231201-1800 #967

iloveeclipse opened this issue Dec 2, 2023 · 14 comments · Fixed by #984
Assignees
Labels
bug Something isn't working regression Regression defect

Comments

@iloveeclipse iloveeclipse added bug Something isn't working regression Regression defect labels Dec 2, 2023
@jukzi
Copy link
Contributor

jukzi commented Dec 4, 2023

@laeubi may that be caused by recent PDE changes?

@laeubi
Copy link
Contributor

laeubi commented Dec 4, 2023

I have no clue, as these tests are not executed as part of the CI build and no one really wants to maintain them and target an unmaintained/deprecated component we probably should simply delete them.

Maybe they need adjustments to new bundles added or adjustments due to P2 changes or updates of dependencies or failed due to changed group ids of JDT who knows ...

@iloveeclipse
Copy link
Member Author

no one really wants to maintain them and target an unmaintained/deprecated component we probably should simply delete them

Sorry, this is not true in both assumptions. The functionality is there, it is used and it is not deprecated, there are people interesting in maintaining it.

But.

Every time release starts, lot of PR's are merged and usually no one cares about broken tests in SDK because most are only interested in pushing their own code and don’t care about SDK as a whole. This is wrong attitude.

There were changes on the pde and p2 and platform. Many of them could break pde.

People who merged into pde and/or p2 or platform should at least check if their changes were not breaking existing tests. Everyone can see current SDK nightly build results. It's not a big deal to check them on the next day after merging PR and ask "hmm, where all the test fails are coming from? Could it be my PR?"

One can't simply expect someone (like me) will always come and cleanup the mess afterwards. Honestly, I'm tired hunting regressions all over SDK code base.

@laeubi
Copy link
Contributor

laeubi commented Dec 4, 2023

Sorry, this is not true in both assumptions. The functionality is there, it is used and it is not deprecated, there are people interesting in maintaining it.

See https://github.com/eclipse-pde/eclipse.pde/blob/master/build/README.md

But.

Don't take me to serious in that regard, I even take some attempts to re enable the tests

but this is currently not my main focus, so if someone has an idea and likes to help out its always welcome...

One can't simply expect someone (like me) will always come and cleanup the mess afterwards.

No one expects that from you, its great you seem to be dedicated to this, thats why I'm always pushing forward things are detected in place when the change happens, if it shows up sometimes after, let it be warnings, javadoc errors test problems, its actually all to late given the low resources split over a large codebase...

@HannesWell
Copy link
Member

One can't simply expect someone (like me) will always come and cleanup the mess afterwards. Honestly, I'm tired hunting regressions all over SDK code base.

This is totally understandable that and I have always been impressed of your diligence in checking the tests and can only say thank you for that.
Hopefully with #980 being merged this does not happen anymore (or at least less, if the cause is not inside PDE).

Now that it is every PDE-contributors burden, does anybody already have an idea what could be the cause?
From the point in time where the failure occured first (it is not completely clear when it started since the build timestamps don't match the tags in the pde all the time), it has to be on of the following changes (assuming the problem is within PDE):
5e9d943...a31752b

Form that the hottest candidate seems to be #906 and given that many failure messages say Premature end of file and that PR changes the way how resources are closed it does not seem to be completely off.

@iloveeclipse
Copy link
Member Author

May be it makes sense to check if test fails from #966 are easier to understand. If something is broken on export, I believe this would directly affect p2 tests that run different bundle/feature build operations.

@merks
Copy link
Contributor

merks commented Dec 5, 2023

I want to echo the thanks for @iloveeclipse tireless work to maintain the quality and integrity of Eclipse.

@jukzi
Copy link
Contributor

jukzi commented Dec 5, 2023

i confirm that when i revert #906 org.eclipse.pde.build.internal.tests.SourceTests.testIndividualSourceBundles_2() stops to fail - i will take a look tomorrow

@jukzi jukzi self-assigned this Dec 5, 2023
@iloveeclipse
Copy link
Member Author

i confirm that when i revert #906 org.eclipse.pde.build.internal.tests.SourceTests.testIndividualSourceBundles_2() stops to fail - i will take a look tomorrow

Could it be, this AntScript that is returned now with closed stream (and it was supposed to be opened):
https://github.com/jukzi/eclipse.pde/blame/b78d8ffa7f250aaa5a90833b1829c9351b470390/build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/AbstractScriptGenerator.java#L255

@jukzi
Copy link
Contributor

jukzi commented Dec 5, 2023

Could it be, this AntScript that is returned now with closed stream (and it was supposed to be opened):

you nailed it

@HannesWell
Copy link
Member

i confirm that when i revert #906 org.eclipse.pde.build.internal.tests.SourceTests.testIndividualSourceBundles_2() stops to fail - i will take a look tomorrow

Could it be, this AntScript that is returned now with closed stream (and it was supposed to be opened): https://github.com/jukzi/eclipse.pde/blame/b78d8ffa7f250aaa5a90833b1829c9351b470390/build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/AbstractScriptGenerator.java#L255

Yep, that used to be open when the method returned. All other changes in pde.build look sane on the other hand.

@jukzi
Copy link
Contributor

jukzi commented Dec 6, 2023

locally PDEBuildTestSuite fails with 58 errors, 4 failures, no matter if i completely revert 906 or not

@iloveeclipse
Copy link
Member Author

locally PDEBuildTestSuite fails with 58 errors, 4 failures, no matter if i completely revert 906 or not

You have to install extra org.eclipse.equinox.executable.feature into your SDK in order to run PDE tests in question, like here

https://github.com/eclipse-platform/eclipse.platform.ui/blob/3665f4d8649e4f3f94257ed9dfa55b512a63c5a5/releng/org.eclipse.ui.releng/platformUiTools.p2f#L180C53-L180C53

Note, the tests are green on latest build, so the problem is solved now: https://download.eclipse.org/eclipse/downloads/drops4/I20231205-1800/testresults/html/org.eclipse.pde.build.tests_ep431I-unit-win32-java17_win32.win32.x86_64_17.html

@mickaelistria
Copy link
Contributor

You have to install extra org.eclipse.equinox.executable.feature into your SDK

Note that it must be installed in the Target Platform actually (which by default is the SDK, but one can create a target platform consisting of their SDK installation + this feature coming from p2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression Regression defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants