Add packager Controller Tests #133

sladyn98 · 2020-08-14T10:36:52Z

This PR adds controller tests for the package generation controller. We already have the package generation class unit tested so this class only makes sure the file is being downloaded and generated correctly

src/test/java/com/org/jenkins/custom/jenkins/distribution/service/PackageControllerTest.java

martinda

These are high level tests. I would prefer to see low level unit tests for methods of each class, IMO it would be easier to review and easier to maintain, specifically unit tests for WarGenerator and PackagerDownloadService. Please provide unit tests for those classes.

src/test/java/com/org/jenkins/custom/jenkins/distribution/service/PackageControllerTest.java

kwhetstone

Left some comments that can be addressed with Martin's review

...a/com/org/jenkins/custom/jenkins/distribution/service/generators/PackageConfigGenerator.java

src/test/java/com/org/jenkins/custom/jenkins/distribution/service/SpringMVCSetup.java

sladyn98 · 2020-08-20T08:33:01Z

These are high level tests. I would prefer to see low level unit tests for methods of each class, IMO it would be easier to review and easier to maintain, specifically unit tests for WarGenerator and PackagerDownloadService. Please provide unit tests for those classes.

As mentioned previously running war generator inside JUNIT results in a lot of dependency issues Oleg tried to resolve some of them but they continued to persist. Hence we cannot write unit tests for these methods

sladyn98 · 2020-08-20T19:27:13Z

src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java

    private final static Logger LOGGER = Logger.getLogger(PackagerController.class.getName());

+    @Autowired
+    public PackagerController(final PackageConfigGenerator packageConfGen, final PackagerDownloadService packagerDownServ) {


Needed to autowire this in order for it to be mocked so that it gets dependency gets injected during testing.

sladyn98 · 2020-08-20T19:27:38Z

src/main/java/com/org/jenkins/custom/jenkins/distribution/service/PackagerController.java

@@ -47,7 +55,7 @@
        String yamlResponse = "";
        HttpStatus httpStatus;
        try {
-            yamlResponse = generatePackageConfig(new JSONObject(postPayload));
+            yamlResponse = packagerConfGen.generatePackageConfig(new JSONObject(postPayload));


PMD complained with short names :)

sladyn98 · 2020-08-20T19:27:59Z

src/main/resources/dummy_war.txt

@@ -0,0 +1 @@
+This will serve as the dummy war file.


Added a text file because the jacoco plugin errored out for dummy war files

sladyn98 · 2020-08-20T19:29:05Z

src/test/java/com/org/jenkins/custom/jenkins/distribution/service/PluginControllerTest.java

@@ -29,23 +31,26 @@ public void setup() {
    @Test
    public void return_status_200() throws Exception {

+        MockMvc pluginMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext).build();


Maven was returning null on running these tests since the web app context was not available in the @before annotation, so injected it in the test.

martinda

Please clean up the class and test method names:

PackageControllerTest.java: must match the Java class name it tests (PackagerController.java)
Test method names: it is best to match the methods they test in the class under test. Here is one example:
1. Class under test method name: getPackageConfig, I expect to see a getPackageConfigTest method for the success path.
2. For the error paths, expand on the name, for example: getPackageConfig404When<condition>
3. When there are multiple error paths, there would be multiple test methods with different s

martinda · 2020-08-23T17:06:14Z

The test methods cover all the conditions. This is good.

kwhetstone · 2020-08-24T05:42:21Z

src/main/java/com/org/jenkins/custom/jenkins/distribution/service/generators/WarGenerator.java

@@ -16,7 +16,7 @@
    private static final String TEMP_PREFIX = "CDS";


-    public static File generateWAR(final String versionName, final String configuration) throws IOException, InterruptedException {
+    public File generateWAR(final String versionName, final String configuration) throws IOException, InterruptedException {


Just question: did you remove static to make it easier to test?

Fix variable name

Add packager Controller Tests

3621cab

sladyn98 requested a review from a team as a code owner August 14, 2020 10:36

Added more tests

096fdd5

sladyn98 changed the title ~~WIP: Add packager Controller Tests~~ Add packager Controller Tests Aug 16, 2020

sladyn98 commented Aug 16, 2020

View reviewed changes

src/test/java/com/org/jenkins/custom/jenkins/distribution/service/PackageControllerTest.java Outdated Show resolved Hide resolved

martinda suggested changes Aug 19, 2020

View reviewed changes

kwhetstone reviewed Aug 20, 2020

View reviewed changes

...a/com/org/jenkins/custom/jenkins/distribution/service/generators/PackageConfigGenerator.java Outdated Show resolved Hide resolved

src/test/java/com/org/jenkins/custom/jenkins/distribution/service/SpringMVCSetup.java Show resolved Hide resolved

Sladyn Nunes added 2 commits August 20, 2020 14:05

Committed suggestions

fbdabac

Add mock tests

29126d1

sladyn98 commented Aug 20, 2020

View reviewed changes

sladyn98 requested review from kwhetstone and martinda August 20, 2020 19:29

sladyn98 linked an issue Aug 23, 2020 that may be closed by this pull request

Unit - Testing for all features #95

Closed

martinda suggested changes Aug 23, 2020

View reviewed changes

kwhetstone reviewed Aug 24, 2020

View reviewed changes

Change test names

4bfc039

sladyn98 requested review from kwhetstone and martinda August 24, 2020 08:12

Sladyn Nunes and others added 3 commits August 24, 2020 13:48

Merge branch 'master' into package-controller

d1ccfcf

Fix variable name

0d0dd8d

Merge pull request #3 from martinda/package-controller

9f68b35

Fix variable name

martinda approved these changes Aug 25, 2020

View reviewed changes

sladyn98 added the test A PR that adds to testing - used by Release Drafter label Aug 25, 2020

sladyn98 merged commit 746fa5b into jenkinsci:master Aug 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add packager Controller Tests #133

Add packager Controller Tests #133

sladyn98 commented Aug 14, 2020 •

edited

Loading

martinda left a comment

kwhetstone left a comment

sladyn98 commented Aug 20, 2020

sladyn98 Aug 20, 2020

sladyn98 Aug 20, 2020

sladyn98 Aug 20, 2020

sladyn98 Aug 20, 2020

martinda left a comment

martinda commented Aug 23, 2020

kwhetstone Aug 24, 2020

sladyn98 Aug 24, 2020

Add packager Controller Tests #133

Add packager Controller Tests #133

Conversation

sladyn98 commented Aug 14, 2020 • edited Loading

martinda left a comment

Choose a reason for hiding this comment

kwhetstone left a comment

Choose a reason for hiding this comment

sladyn98 commented Aug 20, 2020

sladyn98 Aug 20, 2020

Choose a reason for hiding this comment

sladyn98 Aug 20, 2020

Choose a reason for hiding this comment

sladyn98 Aug 20, 2020

Choose a reason for hiding this comment

sladyn98 Aug 20, 2020

Choose a reason for hiding this comment

martinda left a comment

Choose a reason for hiding this comment

martinda commented Aug 23, 2020

kwhetstone Aug 24, 2020

Choose a reason for hiding this comment

sladyn98 Aug 24, 2020

Choose a reason for hiding this comment

sladyn98 commented Aug 14, 2020 •

edited

Loading