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

Further clean-ups and simplifications of smartimport-tests #743

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

HannesWell
Copy link
Member

Generalize redundant code into the template class.

Replace the o.e.pde.ui.tests.smartimport/pom.xml by the still required pom.model. configuration in the build.properties (many properties were obsolete anyway).

@github-actions
Copy link

github-actions bot commented Sep 16, 2023

Test Results

   258 files   -        6     258 suites   - 6   37m 37s ⏱️ - 6m 32s
3 335 tests ±       0  3 304 ✔️ ±       0  30 💤 ±  0  1 ±0 
4 857 runs   - 2 721  4 813 ✔️  - 2 698  42 💤  - 24  2 +1 

For more details on these failures, see this check.

Results for commit ec0a0c0. ± Comparison against base commit 912012e.

This pull request removes 4 and adds 4 tests. Note that renamed tests count towards both.
org.eclipse.ui.tests.smartimport.EclipseJavaProjectTest ‑ testImport
org.eclipse.ui.tests.smartimport.FeatureProjectTest ‑ testImport
org.eclipse.ui.tests.smartimport.PlainEclipseProjectTest ‑ testImport
org.eclipse.ui.tests.smartimport.PlainJavaProjectTest ‑ testImport
AllPDETests org.eclipse.ui.tests.smartimport.ProjectSmartImportTest ‑ testImport[FeatureProject]
AllPDETests org.eclipse.ui.tests.smartimport.ProjectSmartImportTest ‑ testImport[JavaEclipseProject]
AllPDETests org.eclipse.ui.tests.smartimport.ProjectSmartImportTest ‑ testImport[PlainEclipseProject]
AllPDETests org.eclipse.ui.tests.smartimport.ProjectSmartImportTest ‑ testImport[PlainJavaProject]

♻️ This comment has been updated with latest results.

Copy link
Member

@akurtakov akurtakov left a comment

Choose a reason for hiding this comment

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

Now imagine someone to convert it to parameterized test so it becomes single class

@HannesWell
Copy link
Member Author

Now imagine someone to convert it to parameterized test so it becomes single class

Good idea! And since that test-plugin then only contains one class, I think we can just move that test and its resources to the pde.ui.tests Plugin.

@HannesWell HannesWell force-pushed the smartImportTestsCleanup branch 4 times, most recently from b1aa820 to a44cc24 Compare September 16, 2023 14:22
@akurtakov
Copy link
Member

I strongly believe that incremental improvements are better than trying to do everything it once. E.g. push PR that makes this one single class test and leave the move to pde.ui for another PR . Over the years many such "small" improvements got lost after staling in a bigbang change .

@HannesWell
Copy link
Member Author

I strongly believe that incremental improvements are better than trying to do everything it once. E.g. push PR that makes this one single class test and leave the move to pde.ui for another PR . Over the years many such "small" improvements got lost after staling in a bigbang change .

In general I agree, but in this case I had the changes ready locally and just wanted to submit #745 before to not have to adjust the test-suites twice.

For me this is now ready.

Generalize redundant code into the template class.

Replace the o.e.pde.ui.tests.smartimport/pom.xml by the still required
pom.model. configuration in the build.properties (many properties were
obsolete anyway).

Follow-up on "Rewrite smartimport test as plain JUnit test"
And delete the now empty o.e.pde.ui.tests.smartimport.
@laeubi
Copy link
Contributor

laeubi commented Sep 18, 2023

Now imagine someone to convert it to parameterized test so it becomes single class

parameterized JUnit5 test :-)

I think we can just move that test and its resources to the pde.ui.tests Plugin

If its a plain JUnit test you can even move it inside the plugin that is tested into a separate src_test folder and execute if with plain surefire :-)

@HannesWell HannesWell merged commit ec3cb35 into eclipse-pde:master Sep 18, 2023
9 of 14 checks passed
@HannesWell HannesWell deleted the smartImportTestsCleanup branch September 18, 2023 17:07
@HannesWell
Copy link
Member Author

parameterized JUnit5 test :-)

Migrating to JUnit5 is really something I would lime to do (in the near? future).
But can we do that already? IIRC the I-build Tests had some issues with JUnit5, but I could be wrong.

@akurtakov
Copy link
Member

No issue with using JUnit 5 AFAIK. https://github.com/eclipse-platform/eclipse.platform.ui/tree/master/tests/org.eclipse.e4.ui.tests.css.core is one bundle that is exclusively JUnit 5 including @suite usage.

@laeubi
Copy link
Contributor

laeubi commented Sep 19, 2023

IIRC the I-build Tests had some issues with JUnit5, but I could be wrong.

Then probably we should open an issue about the issues to track the possible issue :-)

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