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

Extract ConfigurationSessionTest into separate plug-in #1333

Conversation

HeikoKlare
Copy link
Contributor

The ConfigurationSessionTest introduces a many dependencies to the org.eclipse.core.tests.harness plug-in, as it has to define dependencies to everything that is part of the minimal bundle set for a ConfigurationSessionTest at runtime. The list of dependencies will become even larger when migration session tests to JUnit 5.

In order to reduce the dependencies of org.eclipse.core.tests.harness to what is actually necessary for the plug-in's content, this change moves the ConfigurationSessionTest into a separate plug-in, so that only that plug-in has the according set of dependencies.

Preparation for #1086

The ConfigurationSessionTest introduces a many dependencies to the
org.eclipse.core.tests.harness plug-in, as it has to define dependencies
to everything that is part of the minimal bundle set for a
ConfigurationSessionTest at runtime. The list of dependencies will
become even larger when migration session tests to JUnit 5.

In order to reduce the dependencies of org.eclipse.core.tests.harness to
what is actually necessary for the plug-in's content, this change moves
the ConfigurationSessionTest into a separate plug-in, so that only that
plug-in has the according set of dependencies.
Copy link
Contributor

github-actions bot commented May 1, 2024

Test Results

   639 files  ±0     639 suites  ±0   41m 5s ⏱️ + 2m 25s
 3 949 tests ±0   3 927 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 453 runs  ±0  12 292 ✅ ±0  161 💤 ±0  0 ❌ ±0 

Results for commit b6f6882. ± Comparison against base commit 26d7036.

@HeikoKlare
Copy link
Contributor Author

@HannesWell May I ask you what you think about the extraction of the ConfigurationSessionTest class into a separate plug-in, since you have reworked the class some years ago? The reason is that with the mechanism you introduced to define dependencies of the sessions via referring to classes of a required bundle, these dependencies need to be in the dependencies of org.eclipse.tests.core.harness. However, most of these dependencies are only necessary for setting up configuration session tests and lead to (in most cases unnecessary) transitive dependencies in the users of the test harness plug-in. With a migration to JUnit 5, this list will grow (see #1086) and will make all dependent plug-ins start using JUnit 5 runners at once (which even makes the investigation of differences in the executed tests difficult).

The ConfigurationSessionTest is only used at a single place in platform and at multiple places in Equinox. So if we agreed on this extraction, I would split this PR up into a multi-step migration, first adding the additional plug-in with a dupliate of the ConfigurationSessionTest, then migrating all usages to the new class (in particular in the Equinox code) together with updating the infrastructure (in particular, the SDK test feature) and finally removing the old class.

@HannesWell
Copy link
Member

In general I have no objections, I only think that one extra plugin project only for that class is a bit heavy-weight.
I have not looked in detail in your work, but would it be sufficient to reduce the number of bundles added in addMinimalBundleSet() and just add them in the sub-classes that need them? This could cause some duplicated lines but would probably be less compared to having an extra plugin project, wouldn't it? At the same time it would also be more geared towards the actual needs of a test-case.

@HeikoKlare
Copy link
Contributor Author

Thank you, @HannesWell! I agree that an extra plugin for a single class is quite heavy-weight. Unfortunately, the number of bundles added in addMinimalBundleSet() cannot be reduced, all of them are mandatory. But I just recapped that the reason for all these additional dependencies is new required dependency to org.eclipse.jdt.junit5.runtime, of which all the others are transitive dependencies (see it's plugin's manifest). So it is only about the necessity of making transitive dependencies explicit to add them to the minimal bundle set and not about really adding additional dependencies. I will have a look whether it is better then to accept the additional declaration of these dependencies in the existing plug-in (as the dependencies are there anyway).

@HeikoKlare
Copy link
Contributor Author

I close this and simply add the explicit dependencies to the org.eclipse.core.tests.harness (as they are transitive dependencies anyway). I have resolved the problem introduced with the JUnit 5 runner dependency added to the test harness plug-in and thus being implicitly applied to all dependent test plug-ins via #1334 and #1335.

@HeikoKlare HeikoKlare closed this May 3, 2024
@HeikoKlare HeikoKlare deleted the extract-configurationsessiontest branch May 7, 2024 19:02
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.

2 participants