search

mathworks / jenkins-matlab-plugin

This plugin enables you to run MATLAB® and Simulink® as part of your Jenkins™ build.
45 stars 54 forks source link

Refactor the Jenkins plugin #307

Closed sameagen-MW closed 5 months ago

sameagen-MW commented 6 months ago

Intro

Here is the first draft of the refactor, but there are a few action items that I would like to follow up on with various people. I'll follow up more on this at the end of the review request, but first I'll outline the new architecture:

Architecture

All logic related to interacting with the filesystem or spawning a new process has been moved into the MatlabCommandRunner class.

This class is owned by composition by the three new action classes, RunMatlabCommandAction, RunMatlabBuildAction, and RunMatlabTestsAction. These action classes form the shared logic that is used by both freestyle and pipeline build steps.

the RunMatlab*Builder classes have been moved into a new com.mathworks.ci.freestyle subnamespace, and associated classes used for UI elements have been moved into a com.mathworks.ci.freestyle.options namespace. To facilitate backwards compatibility XStream aliases have been added to all freestyle builders.

The RunMatlab*Step and Matlab*StepExecution classes have been moved to a new com.mathworks.ci.pipeline namespace. Because the step configuration serialization is unaffected by these changes, no aliases are required.

I have moved the existing tests to the src/test/java/integ subfolder, and added a new src/test/java/unit folder to hold the new unit tests. The new unit tests aim to be much smaller in scope than the existing tests and thus hopefully will not increase the runtime of the already quite long running suite.

Future Actions

I modified the existing tests the least possible amout while still attempting to preserve the intention of the test. However, while updating the tests I was unsure of whether a decent number of the tests were still useful. I believe that there is a lot more that can be done to update the existing test suite and potentially remove a few tests that are no longer adding value. I am happy to take that on when I have some spare time, but would like some additional input from others that have more insight into the purpose of each test. If someone else wants to take on the role of looking at and updating the old integ tests I would also be happy with that.

If I don't hear anything back about testing I'll probably reach out directly to @Vahila and @nbhoski and start working on updating the older set of tests sometime in the next month or two!

For qualification, I am pretty confident that these updates will work for all freshly created jobs, but most of my backwards incompatibility checks were done manually so it might be good to have others bash on that as well to make sure edge cases (like the serialized SourceFolders object on a run tests step) still work.

Additionally, I would love if you could check and make sure that all of the functionality that you added for annotating the log in the run build step still works @nikhilbhoski.

Overall breakdown of responsibilities

Because this review is kinda massive, I thought it would be helpful to break it up into slightly smaller pieces to give everyone something to primarily focus their efforts toward.

@Vahila - Updates to existing tests, all new test files. @nikhilbhoski - Updates to RunMatlab*Step.java, Matlab*StepExecution.java, RunMatlab*Builder.java files, RunMatlabBuildAction.java. Especially focusing on maintaing backwards compatibility. Updates to existing test files if you have time. @davidbuzinski - MatlabCommandRunner.java, RunMatlab*Action.java.

These are just some suggestions, so please feel free to review as much as you want, the more the better! I just want to make sure that everything gets checked out by at least one other person, without anyone feeling overwhelmed by the size of the review. Also please take as much time as you need, I don't think anything here is particularly time sensitive!

Thanks again everybody!

sameagen-MW commented 6 months ago

Have to take off a bit early today, I'll get the spotbugs errors fixed tomorrow!

Vahila commented 6 months ago

Intro

Here is the first draft of the refactor, but there are a few action items that I would like to follow up on with various people. I'll follow up more on this at the end of the review request, but first I'll outline the new architecture:

Architecture

All logic related to interacting with the filesystem or spawning a new process has been moved into the MatlabCommandRunner class.

This class is owned by composition by the three new action classes, RunMatlabCommandAction, RunMatlabBuildAction, and RunMatlabTestsAction. These action classes form the shared logic that is used by both freestyle and pipeline build steps.

the RunMatlab*Builder classes have been moved into a new com.mathworks.ci.freestyle subnamespace, and associated classes used for UI elements have been moved into a com.mathworks.ci.freestyle.options namespace. To facilitate backwards compatibility XStream aliases have been added to all freestyle builders.

The RunMatlab*Step and Matlab*StepExecution classes have been moved to a new com.mathworks.ci.pipeline namespace. Because the step configuration serialization is unaffected by these changes, no aliases are required.

I have moved the existing tests to the src/test/java/integ subfolder, and added a new src/test/java/unit folder to hold the new unit tests. The new unit tests aim to be much smaller in scope than the existing tests and thus hopefully will not increase the runtime of the already quite long running suite.

Future Actions

I modified the existing tests the least possible amout while still attempting to preserve the intention of the test. However, while updating the tests I was unsure of whether a decent number of the tests were still useful. I believe that there is a lot more that can be done to update the existing test suite and potentially remove a few tests that are no longer adding value. I am happy to take that on when I have some spare time, but would like some additional input from others that have more insight into the purpose of each test. If someone else wants to take on the role of looking at and updating the old integ tests I would also be happy with that.

If I don't hear anything back about testing I'll probably reach out directly to @Vahila and @nbhoski and start working on updating the older set of tests sometime in the next month or two!

For qualification, I am pretty confident that these updates will work for all freshly created jobs, but most of my backwards incompatibility checks were done manually so it might be good to have others bash on that as well to make sure edge cases (like the serialized SourceFolders object on a run tests step) still work.

Additionally, I would love if you could check and make sure that all of the functionality that you added for annotating the log in the run build step still works @nikhilbhoski.

Overall breakdown of responsibilities

Because this review is kinda massive, I thought it would be helpful to break it up into slightly smaller pieces to give everyone something to primarily focus their efforts toward.

@Vahila - Updates to existing tests, all new test files. @nikhilbhoski - Updates to RunMatlab*Step.java, Matlab*StepExecution.java, RunMatlab*Builder.java files, RunMatlabBuildAction.java. Especially focusing on maintaing backwards compatibility. Updates to existing test files if you have time. @davidbuzinski - MatlabCommandRunner.java, RunMatlab*Action.java.

These are just some suggestions, so please feel free to review as much as you want, the more the better! I just want to make sure that everything gets checked out by at least one other person, without anyone feeling overwhelmed by the size of the review. Also please take as much time as you need, I don't think anything here is particularly time sensitive!

Thanks again everybody!

Hi @sameagen-MW, I can look into the testing aspect and see what tests can be removed/added/updated.

sameagen-MW commented 6 months ago

Intro

Here is the first draft of the refactor, but there are a few action items that I would like to follow up on with various people. I'll follow up more on this at the end of the review request, but first I'll outline the new architecture:

Architecture

All logic related to interacting with the filesystem or spawning a new process has been moved into the MatlabCommandRunner class. This class is owned by composition by the three new action classes, RunMatlabCommandAction, RunMatlabBuildAction, and RunMatlabTestsAction. These action classes form the shared logic that is used by both freestyle and pipeline build steps. the RunMatlab*Builder classes have been moved into a new com.mathworks.ci.freestyle subnamespace, and associated classes used for UI elements have been moved into a com.mathworks.ci.freestyle.options namespace. To facilitate backwards compatibility XStream aliases have been added to all freestyle builders. The RunMatlab*Step and Matlab*StepExecution classes have been moved to a new com.mathworks.ci.pipeline namespace. Because the step configuration serialization is unaffected by these changes, no aliases are required. I have moved the existing tests to the src/test/java/integ subfolder, and added a new src/test/java/unit folder to hold the new unit tests. The new unit tests aim to be much smaller in scope than the existing tests and thus hopefully will not increase the runtime of the already quite long running suite.

Future Actions

I modified the existing tests the least possible amout while still attempting to preserve the intention of the test. However, while updating the tests I was unsure of whether a decent number of the tests were still useful. I believe that there is a lot more that can be done to update the existing test suite and potentially remove a few tests that are no longer adding value. I am happy to take that on when I have some spare time, but would like some additional input from others that have more insight into the purpose of each test. If someone else wants to take on the role of looking at and updating the old integ tests I would also be happy with that. If I don't hear anything back about testing I'll probably reach out directly to @Vahila and @nbhoski and start working on updating the older set of tests sometime in the next month or two! For qualification, I am pretty confident that these updates will work for all freshly created jobs, but most of my backwards incompatibility checks were done manually so it might be good to have others bash on that as well to make sure edge cases (like the serialized SourceFolders object on a run tests step) still work. Additionally, I would love if you could check and make sure that all of the functionality that you added for annotating the log in the run build step still works @nikhilbhoski.

Overall breakdown of responsibilities

Because this review is kinda massive, I thought it would be helpful to break it up into slightly smaller pieces to give everyone something to primarily focus their efforts toward. @Vahila - Updates to existing tests, all new test files. @nikhilbhoski - Updates to RunMatlab*Step.java, Matlab*StepExecution.java, RunMatlab*Builder.java files, RunMatlabBuildAction.java. Especially focusing on maintaing backwards compatibility. Updates to existing test files if you have time. @davidbuzinski - MatlabCommandRunner.java, RunMatlab*Action.java. These are just some suggestions, so please feel free to review as much as you want, the more the better! I just want to make sure that everything gets checked out by at least one other person, without anyone feeling overwhelmed by the size of the review. Also please take as much time as you need, I don't think anything here is particularly time sensitive! Thanks again everybody!

Hi @sameagen-MW, I can look into the testing aspect and see what tests can be removed/added/updated.

Thanks @Vahila!