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 some junit 4 suites to junit 5 #1669

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

carstenartur
Copy link
Contributor

@carstenartur carstenartur commented Sep 21, 2024

What it does

As suggested in #1631 (comment) here we have a first try to update the suites from junit 4 to junit 5 so that step by step upgrade of tests to junit 5 or even a mix of junit 4 and junit 5 based tests is possible.

Unfortunately there are two issues currently:

  1. baseline and build for org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessorUtil.class have different contents
  2. baseline and build for org/eclipse/jdt/internal/corext/fix/StringConcatToTextBlockFixCore$StringBufferFinder.class have different contents

How to test

Author checklist

@carstenartur
Copy link
Contributor Author

@akurtakov can you have a look if this is what you suggested?

@carstenartur carstenartur marked this pull request as ready for review September 22, 2024 08:37
@akurtakov
Copy link
Contributor

I haven't done detailed review but this is aligning with the overall idea - make suites JUnit5 ones so individual tests can be added/migrated to JUnit 5 later.

@@ -124,6 +126,12 @@ public static void setUpTest() {
public void setUp() throws Exception {
fProject= pts.getProject();

Map<String, String> options= fProject.getOptions(false);
Copy link
Contributor

@akurtakov akurtakov Sep 24, 2024

Choose a reason for hiding this comment

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

Is this on purpose here? This hardcoded 1.8 compliance here doesn't seem correct to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in despair because of the build issues. Can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to delete the file from the patch. In gerrit it was possible. Hmm, should stay away from this while accessing it from a mobile phone. Sorry

@akurtakov
Copy link
Contributor

Same number of tests, tests succeed, only a small portion of new code which looks wrong to me.
@jjohnstn would you please double check this PR?

…/FatJarExportTests.java

Delete accidently changed file
@@ -1,1017 +0,0 @@
/*******************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

You should have deleted your change only, not the whole file.

@jjohnstn
Copy link
Contributor

Hi @akurtakov I did look at it and was waiting for your comments as @carstenartur was asking if it met your expectations. I too noticed that same number of tests ran and succeeded. Does this patch make it possible to have the explicit encoding tests added instead of forcing manual execution?

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