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

Enable pde build tests #905

Closed
wants to merge 2 commits into from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Nov 10, 2023

At least the test runtime comes up already ...

Even though no search path is provided we need to decode the URL and see
if we can find the schema in the current workspace.

Also fixes an issue where schema can't be opened by double click on
linux.

Fix eclipse-pde#890
@laeubi laeubi requested a review from HannesWell November 10, 2023 12:25
@laeubi laeubi force-pushed the enable_pde_build_tests branch from 6ef2284 to 55d881a Compare November 10, 2023 12:25
Copy link

github-actions bot commented Nov 10, 2023

Test Results

     270 files  ±0       270 suites  ±0   45m 26s ⏱️ - 12m 12s
  3 327 tests ±0    3 297 ✔️ ±0  30 💤 ±0  0 ±0 
10 278 runs  ±0  10 188 ✔️ ±0  90 💤 ±0  0 ±0 

Results for commit baa56b4. ± Comparison against base commit eeffb96.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member

Enabling them is a good idea, but before this is submitted the tests should of course pass.

So fixing #884 is a prerequisite.

@HannesWell HannesWell force-pushed the enable_pde_build_tests branch from 55d881a to 37a9bca Compare November 11, 2023 17:12
@laeubi laeubi force-pushed the enable_pde_build_tests branch 2 times, most recently from f6dcd30 to c7eeb58 Compare November 20, 2023 06:21
Comment on lines +27 to +51
<build>
<plugins>
<plugin>
<groupId>org.eclipse.tycho</groupId>
<artifactId>tycho-surefire-plugin</artifactId>
<configuration>
<dependencies>
<dependency>
<type>eclipse-plugin</type>
<artifactId>org.eclipse.osgi.compatibility.state</artifactId>
<version>0.0.0</version>
</dependency>
<dependency>
<type>p2-installable-unit</type>
<artifactId>org.eclipse.equinox.executable</artifactId>
<version>0.0.0</version>
</dependency>
<dependency>
<type>eclipse-feature</type>
<artifactId>org.eclipse.sdk</artifactId>
<version>0.0.0</version>
</dependency>
</dependencies>
<testRuntime>p2Installed</testRuntime>
</configuration>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary to have it installed at that stage (my concern is that test execution in requirements aren't so portable)?
Could it be replaced by some instructions in p2.inf?
Could it be replaced by creation of a TargetPlatform in the tests that would consist of running target?
Do you really need it as "p2installed", or would running this setting the right testProduct/testApplication to usual SDK values be sufficient? p2installed is usually useful only when there are crazy things like touchpoints or when you need the files to be organized on disk similarly to an installation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary to have it installed at that stage (my concern is that test execution in requirements aren't so portable)?

I tried without but didn't work so well... I have no clue why the compatibility is not discovered automatically but it seems a common problem.

Could it be replaced by some instructions in p2.inf?

Probably yes, but last time I tried it wasn't that obvious and introduced build cycles in aggregator.

Could it be replaced by creation of a TargetPlatform in the tests that would consist of running target?

The problem is that the test don't start at all, the only good think is that one can easily try this standalone and play around by go into the folder eclipse.pde/build/org.eclipse.pde.build.tests and execute mvn clean verify and it is also quite fast.

Do you really need it as "p2installed"

p2installed was just something I tried here because the test do special stuff, especially require that the plugins are exploded, that an installation location is set and so on, I haven't debugged this in detail and there might be mitigations but as pde-build is nothing I have experience with its hard for me to decide.

p2installed is usually useful only when there are crazy things like touchpoints or when you need the files to be organized on disk similarly to an installation.

I fear that's the case here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no clue why the compatibility is not discovered automatically but it seems a common problem.

It's a fragment of org.eclipse.osgi, and it's required whenever one invokes Platform.getPlatformAdmin().

I added it (in p2.inf) and see that it indeed seems to require a full installation (using the simpleconfigurator/bundles.info file).

But I see the text.xml file contains 2 launches, one with a system property. Passing this system property may help running the tests here.
I'm actually curious as to whether PDE would handle this case as well without enabling p2.

@mickaelistria
Copy link
Contributor

I've opened #927 with further configuration to run the tests. I get 12 failures locally that I believe are all caused by the missing "Delta Pack".
Provisionign a "delta pack" shouldn't be a test pre-requisite, but instead be somehting the the test themselves would perform as part of their setup (in Java code), so we can run the tests with whichever build or script technology without having to rewrite those bits. I will try to work on it sometime later, but if anyone is willing to start this (not too difficult I hope) change in the meantime, it would be welcome and can be developed/merged independently from this current PR.

@laeubi
Copy link
Contributor Author

laeubi commented Nov 21, 2023

@mickaelistria thanks for that, I have added a comment in the PR regarding delta-pack.

@laeubi
Copy link
Contributor Author

laeubi commented Nov 21, 2023

Closing in favor of

@laeubi laeubi closed this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants