Migrate Junit4to5

[aamotharald]

Hi @MatKuhr

I performed a migration from JUnit4 to jUnit5 to get rid of any legacy / technical debt with regards to the testing framework.

I used an automated migration tool (OpenRewrite) and then had to deal manually with a @nested test and the tests for the maven plugin mojos.

• Functionality scope stated & covered
• Tests cover the scope above
• Error handling created / updated & covered by the tests above~
• Documentation updated
• Release notes updated

Please review CAREFULLY the changes made to the Mojo's which is actually a change in the production code to make the mojo tests pass with the JUnit5 approach.
All other changes are test code changes by keeping scope and the number of tests exactly the same. I was even able to enable a test that was deactivated before.

[MatKuhr]

Hi @aamotharald, thanks for your contribution!

Unfortunately, this approach doesn't work, because the AbstractMojoTestCase extends junit.framework.TestCase which is a JUnit 4 class. We had tried to get rid of JUnit 4 before, but didn't manage to do it within reasonable efforts. However, with the current solution tests are fully executed using the JUnit 5 runner. The JUnit 4 dependency is only declared to prevent version conflicts, not for executing tests.

Aside from that, I don't understand why the resilience tests got changed, they are already using JUnit 5 and thus should have remain unchanged?

Finally, maybe you can split out the improvements to productive code, then it'll be much easier to understand them + the related test changes, compared to JUnit related test changes.

[aamotharald]

Hi @MatKuhr ,

still formatting ..

I see so the maven-plugin-testing-harness pulls in JUnit4.

At least with this restructuring once the testing-harness starts to support JUnit5 the switch will be done directly as I got rid of the JUnit4 rules which are not supported anymore in JUnit5 .

[aamotharald]

Hi @MatKuhr ,

Aside from that, I don't understand why the resilience tests got changed, they are already using JUnit 5 and thus should have remain unchanged?
I have removed the resilience tests changes. The automatic refactoring did something strage (added the @nested Annotation) which the required some changes on top.

Here marked the changes to production code with regards to Mojo parameters initialization:
https://github.com/SAP/cloud-sdk-java/pull/646/files#diff-4003e606fc15367c1dc753ab726025e50332e1b007c1c17b40d8378bf66a736b
https://github.com/SAP/cloud-sdk-java/pull/646/files#diff-c045461f9e02233bbf4fc2bb0762a3bb30247ca140c4f3c09289264d90ff9f87
https://github.com/SAP/cloud-sdk-java/pull/646/files#diff-a7728b1c4dcc9b610c4ca0d80a0ac53e9edbca165fc46a494051624b95bcc0d2
It seems to me that due to the @parameter Annotation dealing with null values is only needed for tests, as in production the default value will get applied and null can't be set for the MoJo..
E.g.: @Parameter( property = "openapi.generate.skip", defaultValue = "false" ) is not applied for test execution ..

The only advantage I see in this change here is that junit4 does not get pulled explicitly, but transitively.
That means that future changes in the maven-plugin-testing-harness will not case dependency incompatibility issues.
Looking at the activity around the repo (last change 2014) I doubt that the testing support in maven-plugin-testing-harness will soon also offer an abstract Base class based on JUnit 5..

[aamotharald]

Seems like maven-plugin-testing-harness 4.x.x is going for JUnit5 support with a JUnit5 Extension instead of inheriting from a base class.

@ExtendWith(MojoExtension.class)

Hence once we have a non-beta 4.0.0 release the tests would have to get refactored accordingly anyhow :-(

[aamotharald]

Hi @MatKuhr ,

I did some more research and think it makes sense to close my PR.

I have requested in ths ASF JIRA to have the testing-harness 3.3.4 released which offers JUnit5 support.

Then the plugin testing could be done like this:

package com.sap.cloud.sdk.datamodel.odata.generator;

import com.sap.cloud.sdk.datamodel.odata.generator.annotation.DefaultAnnotationStrategy;
import com.sap.cloud.sdk.datamodel.odata.utility.NameSource;
import com.sap.cloud.sdk.datamodel.odata.utility.S4HanaNamingStrategy;
import org.apache.maven.plugin.logging.Log;
import org.apache.maven.plugin.testing.junit5.InjectMojo;
import org.apache.maven.plugin.testing.junit5.MojoTest;
import org.junit.jupiter.api.Test;

import javax.inject.Inject;

import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.junit.jupiter.api.Assertions.assertEquals;

@MojoTest
public class DataModelGeneratorMojoJunit5Test {

    private static final String POM = "src/test/resources/DataModelGeneratorMojoTest/pom-junit5.xml";

    @Inject
    private Log log;

    @Test
    @InjectMojo(goal = "generate", pom = POM)
    void simpleMojo(DataModelGeneratorMojo mojo) {
        assertEquals(log, mojo.getLog());
        final DataModelGenerator generator = mojo.getDataModelGenerator();

        assertSoftly(softly -> {
            softly.assertThat(generator.getInputDirectory().getName()).isEqualTo("myInputDir");
            softly.assertThat(generator.getOutputDirectory().getName()).isEqualTo("myOutputDir");
            softly.assertThat(generator.isDeleteTargetDirectory()).isTrue();
            softly.assertThat(generator.isForceFileOverride()).isTrue();
            softly.assertThat(generator.getPackageName()).isEqualTo("my.package");
            softly.assertThat(generator.getDefaultBasePath()).isEqualTo("my/base/path/");
            softly
                    .assertThat(generator.getServiceNameMappings().getName())
                    .isEqualTo("myServiceNameMappings.properties");
            softly
                    .assertThat(generator.getNamingStrategy().getClass().getName())
                    .isEqualTo(S4HanaNamingStrategy.class.getName());
            softly.assertThat(generator.getNameSource()).isEqualTo(NameSource.NAME);
            softly
                    .assertThat(generator.getAnnotationStrategy().getClass().getName())
                    .isEqualTo(DefaultAnnotationStrategy.class.getName());
            softly.assertThat(generator.isGeneratePojosOnly()).isTrue();
            softly.assertThat(generator.getExcludeFilePattern()).isEqualTo("**/myExclusions/**");
            softly.assertThat(generator.isGenerateLinksToApiBusinessHub()).isTrue();
            softly.assertThat(generator.getIncludedEntitySets()).contains("entitySet1", "entitySet2");
            softly.assertThat(generator.getIncludedFunctionImports()).contains("fnImport1", "fnImport2", "fnImport3");
            softly.assertThat(generator.isFailOnWarning()).isTrue();
            softly.assertThat(generator.getCopyrightHeader()).isEmpty();
            softly.assertThat(generator.isServiceMethodsPerEntitySet()).isTrue();
        });    }

}

What do you think?

[MatKuhr]

Thanks for requesting the update! Yeah, I think that would be the best solution for us 👍🏻 Then we can simply get rid of the current workaround and fully remove junit4 from our dependency tree.

Since you have access to that project, could you maybe ping here / re-open this PR once the update is available? Otherwise our dependabot should also catch the update eventually