Adding system tests #399

Vahila · 2025-01-20T16:05:40Z

This PR has following changes:

Added system tests under src/test/java/com/mathworks/ci/systemTests
Moved the integ test under the above mentioned folder as sometimes there are errors because of inconsistent directory and package path
Deleted some unnecessary tests

System tests needs access to MATLAB to run successfully. The MATLAB root path is read from 'MATLAB_ROOT' env variable. These tests are configured to run only if the 'MATLAB_ROOT' variable is defined. This is to ensure that the tests pass in Jenkinsci jobs and it makes it easier to run the tests locally as well.

Matrix project needs two versions of MATLAB to run successfully. Since there is an error while using Set up MATLAB twice, these tests would be skipped(Updating the main.yml to define MATLAB_ROOT_22b would trigger the tests)

System tests follow the standard integration tests format *IT.java. These tests can be run in isolation using failsafe plugin.
mvn test - Runs only unit tests
mvn verify - Runs both unit and system tests
mvn failsafe:integration-test - Runs only system tests

…tlab-plugin into integ_tests

* Delete unnecessary tests * Move some tests into 'systemTests' folder

davidbuzinski · 2025-01-23T16:25:23Z

pom.xml

+		<!-- Dependencies for system tests -->
+		<!-- https://mvnrepository.com/artifact/com.jcabi/jcabi-xml -->
+		<dependency>
+			<groupId>com.jcabi</groupId>
+			<artifactId>jcabi-xml</artifactId>
+			<version>0.23.1</version>
+		</dependency>
+		<!-- https://mvnrepository.com/artifact/org.jenkinsci.plugins/pipeline-model-definition -->
+		<dependency>
+			<groupId>org.jenkinsci.plugins</groupId>
+			<artifactId>pipeline-model-definition</artifactId>
+		</dependency>
+		<!-- Need unzip to add the files in pipeline workspace -->
+		<!-- https://mvnrepository.com/artifact/org.jenkins-ci.plugins/pipeline-utility-steps -->
+		<dependency>
+			<groupId>org.jenkins-ci.plugins</groupId>
+			<artifactId>pipeline-utility-steps</artifactId>
+			<version>2.7.1</version>
+			<scope>test</scope>
+		</dependency>
+		<!-- TODO: see if this is necessary Using for verifyCodeCoverage -->
+		<!-- https://mvnrepository.com/artifact/org.jenkins-ci.plugins/cobertura -->
+		<dependency>
+			<groupId>org.jenkins-ci.plugins</groupId>
+			<artifactId>cobertura</artifactId>
+			<version>1.16</version>
+		</dependency>


I see the pipeline-utility-steps dependency uses the test scope (<scope>test</scope>), can all of them use test scope?

davidbuzinski · 2025-01-23T16:33:18Z

src/test/java/com/mathworks/ci/actions/MatlabActionTest.java

@@ -1,136 +1,136 @@
-package unit.com.mathworks.ci.actions;


I never noticed this before but we have separate test packages. In the Bamboo plugin we use the same package. Should the package be updated or are we going to keep them in separate packages?

davidbuzinski · 2025-01-23T16:41:48Z

.github/workflows/main.yml

@@ -0,0 +1,28 @@
+name: using mpm to install MATLAB


I like that we're using GitHub Actions. Does this mean we can remove the Azure Pipelines yaml?

davidbuzinski · 2025-01-23T16:47:03Z

.github/workflows/main.yml

+    steps:
+      - name: Check out repository
+        uses: actions/checkout@v2
+
+      - name: Setup Java 11
+        uses: actions/setup-java@v3
+        with:
+          java-version: '11'
+          distribution: 'temurin'
+


Consider updating to actions/checkout@v4 and actions/setup-java@v4

Also consider adding the cache: maven option to the setup-java step as shown here. It could improve build times by quite a bit.

davidbuzinski · 2025-01-23T17:04:06Z

src/test/java/com/mathworks/ci/systemTests/GlobalToolIT.java

+        String matlabRoot = System.getenv("MATLAB_ROOT");
+        Assume.assumeTrue("Not running tests as MATLAB_ROOT environment variable is not defined", matlabRoot != null && !matlabRoot.isEmpty());


Is MATLAB_ROOT getting set by our Jenkins plugin? It doesn't seem like this would be set before the test class is run.

davidbuzinski · 2025-01-23T18:45:43Z

src/test/java/com/mathworks/ci/systemTests/GlobalToolIT.java

+        String matlabRoot = System.getenv("MATLAB_ROOT");
+        String matlabRoot22b = System.getenv("MATLAB_ROOT_22b");
+        Assume.assumeTrue("Not running tests as MATLAB_ROOT_22b environment variable is not defined", matlabRoot22b != null && !matlabRoot22b.isEmpty());
+
+        Utilities.setMatlabInstallation("MATLAB_PATH_1", matlabRoot, jenkins);
+        Utilities.setMatlabInstallation("MATLAB_PATH_2", matlabRoot22b, jenkins);


Same question here. What's setting MATLAB_ROOT_22b environment variable?

sameagen-MW

Looks really good! Definitely a huge improvement, just a couple of things that I caught during the review.

sameagen-MW · 2025-01-24T14:42:30Z

.github/workflows/main.yml

+name: using mpm to install MATLAB
+on: push
+jobs:
+  my-job:


Could maybe give this job a more descriptive name in case we want to add more in the future.

sameagen-MW · 2025-01-24T14:54:08Z

src/test/java/com/mathworks/ci/systemTests/GlobalToolIT.java

+    private UseMatlabVersionBuildWrapper buildWrapper;
+
+    @Rule
+    public Timeout timeout = Timeout.seconds(0);


Timing out after 0 seconds seems pretty quick to me, was that intentional?

sameagen-MW · 2025-01-24T15:01:23Z

src/test/java/com/mathworks/ci/systemTests/GlobalToolIT.java

+        Utilities.setMatlabInstallation("MATLAB_PATH", Utilities.getMatlabRoot(), jenkins);
+
+        // Selecting MATLAB that is defined as Global tool
+        MatlabInstallation _inst = MatlabInstallation.getInstallation("MATLAB_PATH");


I normally think of variables names starting with an _ as values that are going to be thrown away or not referenced later. Maybe we should just call that variable inst?

sameagen-MW · 2025-01-24T15:04:12Z

src/test/java/com/mathworks/ci/systemTests/GlobalToolIT.java

+
+        jenkins.assertLogContains(_inst.getHome(), build);
+        jenkins.assertBuildStatus(Result.SUCCESS, build);
+        jenkins.assertLogContains("apple", build);


I think that we still print out the contents of the script that we're generating, so checking for apple in the build log might not indicate that the command actually ran. It could just indicate that a script with those contents were generated.

sameagen-MW · 2025-01-24T15:05:16Z

src/test/java/com/mathworks/ci/systemTests/GlobalToolIT.java

+        FreeStyleBuild build = project.scheduleBuild2(0).get();
+
+        jenkins.assertBuildStatus(Result.FAILURE, build);
+        jenkins.assertLogContains("Verify global tool configuration for the specified node.", build);


This could fail in a future Jenkins update, which is just something that we should be aware of in case it ever shows up.

sameagen-MW · 2025-01-24T16:00:26Z

src/test/java/com/mathworks/ci/systemTests/RunMatlabBuildIT.java

+    @Test
+    public void verifyMATLABscratchFileNotGenerated() throws Exception {
+        this.buildWrapper.setMatlabBuildWrapperContent(
+                new MatlabBuildWrapperContent(Message.getValue("matlab.custom.location"), Utilities.getMatlabRoot()));
+        project.getBuildWrappersList().add(this.buildWrapper);
+        scriptBuilder.setTasks("");
+        project.getBuildersList().add(this.scriptBuilder);
+        FreeStyleBuild build = project.scheduleBuild2(0).get();
+        File matlabRunner = new File(build.getWorkspace() + File.separator + "runMatlabTests.m");
+        Assert.assertFalse(matlabRunner.exists());
+    }


I think this test case is probably not necessry.

sameagen-MW · 2025-01-24T16:01:04Z

src/test/java/com/mathworks/ci/systemTests/RunMatlabBuildIT.java

+        RunMatlabBuildBuilder tester =
+                new RunMatlabBuildBuilder();


sameagen-MW · 2025-01-24T16:01:24Z

src/test/java/com/mathworks/ci/systemTests/RunMatlabBuildIT.java

+        RunMatlabBuildBuilder tester =
+                new RunMatlabBuildBuilder();


sameagen-MW · 2025-01-24T16:03:04Z

src/test/java/com/mathworks/ci/systemTests/StartupOptionsIT.java

+        runTestsStep.setStartupOptions(new StartupOptions("-logfile outputTests.log"));
+        project.getBuildersList().add(runTestsStep);
+
+        //Rub Build step


Suggested change

//Rub Build step

// Run Build step

sameagen-MW · 2025-01-24T16:05:01Z

src/test/java/com/mathworks/ci/systemTests/UseMATLABVersionIT.java

+// ToDo: Figure out how to run these tests where MATLAB needs to be unaccesible locally and in CI/CD
+
+//    @Test
+//     public void verifyEmptyRootError() throws Exception {
+//         String script = "pipeline {\n" +
+//                         "  agent any\n" +
+//                         "    stages{\n" +
+//                         "        stage('Run MATLAB Command') {\n" +
+//                         "            steps\n" +
+//                         "            {\n" +
+//                         "              runMATLABCommand 'version' \n" +
+//                         "            }\n" +
+//                         "        }\n" +
+//                         "    }\n" +
+//                         "}";
+//        WorkflowJob project = jenkins.createProject(WorkflowJob.class);
+//        project.setDefinition(new CpsFlowDefinition(script, true));
+//        WorkflowRun build = project.scheduleBuild2(0).get();
+//
+//         jenkins.assertLogContains("MATLAB_ROOT",build);
+//         jenkins.assertBuildStatus(Result.FAILURE,build);
+//     }


Comment left in

Vahila added 24 commits December 24, 2024 12:38

Refactor existing tests

122d4da

Add system tests for Run MATLAB Command step

95fabb5

Set up main.yml to run system tests

db9eb7c

Update maven goal

42a5d44

Update version

60e88f8

Update test set up

9e9f57c

Add tests for Matrix project

309b23d

Add tests for Run MATLAB Tests build step

42730cc

Add tests for Artifacts generation

b8921ad

Add tests for Customize Test Run

2ce87ae

Add tests for MATLAB Global Tool

dc4aab5

Add tests for Start Up options

21763ac

Add test data

62dcd17

Update main.yml

71a7d4c

Add tests for MPMinstall feature

e96379a

Update FilterTestData.zip

1e85a8f

Use Java 11 in set up

14276c4

Add tests for Run MATLAB Build step

0140125

Refactor tests

eba0e3f

Use mvn verify

a52b697

Update failing test

53be112

Merge branch 'integ_tests' of https://github.com/mathworks/jenkins-ma…

e0d7182

…tlab-plugin into integ_tests

Delete MPMInstallIT.java

bcb085d

Refactor tests

de3da63

* Delete unnecessary tests * Move some tests into 'systemTests' folder

Vahila requested review from davidbuzinski, sameagen-MW and mw-kapilg January 20, 2025 16:10

Vahila marked this pull request as ready for review January 20, 2025 16:18

davidbuzinski reviewed Jan 23, 2025

View reviewed changes

sameagen-MW reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding system tests #399

Adding system tests #399

Vahila commented Jan 20, 2025 •

edited

Loading

davidbuzinski Jan 23, 2025

davidbuzinski Jan 23, 2025

davidbuzinski Jan 23, 2025

davidbuzinski Jan 23, 2025

davidbuzinski Jan 23, 2025

davidbuzinski Jan 23, 2025

sameagen-MW left a comment

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

sameagen-MW Jan 24, 2025

		String matlabRoot = System.getenv("MATLAB_ROOT");
		Assume.assumeTrue("Not running tests as MATLAB_ROOT environment variable is not defined", matlabRoot != null && !matlabRoot.isEmpty());

Adding system tests #399

Are you sure you want to change the base?

Adding system tests #399

Conversation

Vahila commented Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameagen-MW left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vahila commented Jan 20, 2025 •

edited

Loading