-
Notifications
You must be signed in to change notification settings - Fork 54
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
Adding system tests #399
Adding system tests #399
Conversation
* Delete unnecessary tests * Move some tests into 'systemTests' folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! Definitely a huge improvement, just a couple of things that I caught during the review.
src/test/java/com/mathworks/ci/systemTests/RunMatlabBuildIT.java
Outdated
Show resolved
Hide resolved
src/test/java/com/mathworks/ci/systemTests/RunMatlabBuildIT.java
Outdated
Show resolved
Hide resolved
src/test/java/com/mathworks/ci/systemTests/RunMatlabBuildIT.java
Outdated
Show resolved
Hide resolved
src/test/java/com/mathworks/ci/systemTests/StartupOptionsIT.java
Outdated
Show resolved
Hide resolved
src/test/java/com/mathworks/ci/systemTests/UseMATLABVersionIT.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really clean!
public void verifyGlobalToolScriptedPipeline() throws Exception { | ||
Utilities.setMatlabInstallation("MATLAB_PATH_1", Utilities.getMatlabRoot(), jenkins); | ||
String script = "node {\n" + | ||
" def matlabver\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this definition here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are talking about 'setMATLABInstallation' this is equivalent to defining MATLAB as global tool. As in the test we are using "matlab 'MATLAB_PATH_1'", we need to define the tool first.
// 'test' task fails | ||
jenkins.assertBuildStatus(Result.FAILURE, build); | ||
jenkins.assertLogContains("buildtool check test dummy", build); | ||
jenkins.assertLogNotContains("In check task", build); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat curious why the log shouldn't contain the output from the check
task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test data is set so that the 'test' task fails(as source folder is not added to path). This is so that we have a task that fails which can used in testing different scenarios
This PR has following changes:
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