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

Migrate tests in org.eclipse.test.performance to JUnit 5 #1454

Merged

Conversation

HeikoKlare
Copy link
Contributor

The tests in org.eclipse.tests.performance are still written in JUnit 3, i.e., the test classes extend TestCase.

This change

  • migrates tests and test suite to JUnit 5, and
  • adds the missing SimplePerformanceMeterTest to the according test suite.

Note that these tests are not automatically executed at all, as they are neither part of a test plug-in nor does the plug-in seem to be configured to execute the tests via surefire or some ant script. But they run well when started from within the IDE. Making them executed in a CI build may be a follow-up task.

The tests in org.eclipse.tests.performance are still written in JUnit 3,
i.e., the test classes extend TestCase.

* Migrates tests and test suite to JUnit 5.
* Adds the missing SimplePerformanceMeterTest to
the according test suite
@Phillipus
Copy link

Note that these tests are not automatically executed at all

If you're trying to run the Suite using Tycho, see:

eclipse-tycho/tycho#2462

@HeikoKlare
Copy link
Contributor Author

Note that these tests are not automatically executed at all

If you're trying to run the Suite using Tycho, see:

eclipse-tycho/tycho#2462

Thanks for the pointer, @Phillipus! I was not aware that @Suite may not be properly handled in Tycho builds. I don't think that is a problem for this PR, as the test will not be executed via Tycho anyway, but interesting to know for further migrations.

@HeikoKlare HeikoKlare marked this pull request as ready for review October 20, 2023 09:43
@HannesWell
Copy link
Member

Its nice to see some modernization of the tests.
Just in case you didn't already do it, can you please check if the test classes that now don't extend TestCase anymore are not extended anywhere else in the platform.

@HeikoKlare
Copy link
Contributor Author

Yes, I have checked that in the platform and JDT projects no subclasses of those I have changed exist. But since the tests are placed in and exported from an ordinary plug-in that is part of the Eclipse test framework feature, they might be used/extended anyway. So taking semantic versioning serious, these changes would actually require a major version bump of the plug-in. On the other hand, I cannot image that anyone extends such specific test classes.

@HannesWell
Copy link
Member

Yes, I have checked that in the platform and JDT projects no subclasses of those I have changed exist.

Thanks for checking.

But since the tests are placed in and exported from an ordinary plug-in that is part of the Eclipse test framework feature, they might be used/extended anyway. So taking semantic versioning serious, these changes would actually require a major version bump of the plug-in. On the other hand, I cannot image that anyone extends such specific test classes.

Theoretically that would be possible, but I assume that a Performance-Test-Plugin is not referenced by any 'production' Plugin. Maybe somebody external uses it as base for own tests, but in that case tests can be adjusted.
@akurtakov do you have some more definitive knowledge?

@akurtakov
Copy link
Member

Performance test plugins are not referenced by anything in the SDK AFAIK.
Unfortunately they no longer execute for some reason too . E.g. one can see https://download.eclipse.org/eclipse/downloads/drops4/R-4.28-202306050440/performance/performance.php (half there) but for all 4.29 and newer builds these are not there https://download.eclipse.org/eclipse/downloads/drops4/I20231020-1800/performance/performance.php and no one has looked at the results for years
Still, streamlining these test is nice if someone wants to revive the performance analysis it would be easier to grasp the code itself, the infrastructure to run them though would better be seriously rethought.

@akurtakov
Copy link
Member

Let's merge as the change is good from my POV.

@akurtakov akurtakov merged commit ba25cf0 into eclipse-platform:master Oct 22, 2023
2 of 4 checks passed
@HeikoKlare
Copy link
Contributor Author

Thanks, @HannesWell and @akurtakov!
A follow-up issue has been detected in eclipse-equinox/equinox#386 and should be fixed via eclipse-platform/eclipse.platform#758.

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.

4 participants