initial functional tests commit #185
Conversation
.vscode/settings.json
Outdated
There was a problem hiding this comment.
Should this IDE settings file be included?
There was a problem hiding this comment.
Is this file needed? It's a Java class file with a .txt extension so may be a temporary file. If it is needed then there's a number of commented out lines of code in it that will need to be looked at.
| @@ -0,0 +1,23 @@ | |||
| package functions; | |||
There was a problem hiding this comment.
Generally speaking, the package structure should mirror that of the main source classes. Store the functional test classes under src/functionalTest/java/uk/gov/moj/sdt so that the package for this class becomes uk.gov.moj.sdt.functions.
The same comment applies to all the other classes.
| import net.thucydides.core.annotations.Step; | ||
| import io.restassured.RestAssured; | ||
| import io.restassured.response.Response; | ||
|
|
||
| import java.nio.file.Files; | ||
| import java.nio.file.Paths; |
There was a problem hiding this comment.
Organise these imports. If you're using Visual Studio Code the shortcut is option + shift + o (assuming you are using a Mac).
|
|
||
|
|
There was a problem hiding this comment.
Remove one of these blank lines.
| String xmlContent = ""; | ||
| try { | ||
| xmlContent = new String(Files.readAllBytes(Paths.get(getClass().getClassLoader().getResource("statusUpdate.xml").toURI()))); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
This code for reading a file is duplicated in the IssueWarrant and MCOLJudgment classes. It would be better to put it in a method in a separate utility class and call that instead.
In the event of an exception being raised it would be better to fail the test rather than handling it and carrying on as is done here. This would make it easier to debug. In JUnit you can explicitly fail a test using org.junit.jupiter.api.Assertions.fail - I don't know if Serenity offers an equivalent. If not you could just let the method throw the exception by adding throws <exception class name> to the signature.
| e.printStackTrace(); | ||
| } | ||
|
|
||
| randomNumber = (int)(Math.random() * 10000); // Generate a random number |
There was a problem hiding this comment.
Use the XMLFunctions.generateRandomNumber() method here rather than duplicating the code in it.
| public String removeNamespaces(String xmlResponse) { | ||
| return xmlResponse.replaceAll("xmlns.*?(\"|\').*?(\"|\')", "") | ||
| .replaceAll("(<)(\\w+:)(.*?>)", "$1$3") | ||
| .replaceAll("(</)(\\w+:)(.*?>)", "$1$3"); | ||
| } | ||
|
|
||
| public String getCleanResponse() { | ||
| String xmlResponse = response.asString(); | ||
| xmlResponse = removeNamespaces(xmlResponse); | ||
| return xmlResponse; | ||
| } |
There was a problem hiding this comment.
These methods don't appear to be used and are duplicates of those on the XMLFunctions class. Remove them from this class.
There was a problem hiding this comment.
Organise the imports on this class (see comment for ClaimStatusSteps class).
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; |
There was a problem hiding this comment.
These are JUnit4 classes. Replace them with the JUnit5 equivalents:
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
More details about replacing @RunWith with @ExtendWith can be found in the comment for that line.
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.matchesRegex; | ||
| import static org.hamcrest.CoreMatchers.containsString; | ||
| import org.junit.jupiter.api.Assertions; |
There was a problem hiding this comment.
Replace this with import static org.junit.jupiter.api.Assertions.assertEquals. You'll then be able to remove Assertions. from before assertEquals() on line 34.
| import steps.ClaimStatusSteps; | ||
|
|
||
|
|
||
| @RunWith(SerenityRunner.class) |
There was a problem hiding this comment.
This should be replaced with the JUnit5 equivalent which is @ExtendWith(SerenityJUnit5Extension.class)
|
|
||
|
|
There was a problem hiding this comment.
Remove one of these blank lines.
| @Steps | ||
| ClaimStatusSteps claimStatusSteps; | ||
|
|
||
| @Before |
There was a problem hiding this comment.
Change this to @BeforeEach (the JUnit5 equivalent).
|
|
||
| @Test | ||
| public void claimStatusUpdateResponseCode() { | ||
| XmlPath xmlPath = new XmlPath(claimStatusSteps.getCleanResponse()).setRoot("Envelope.Body.bulkResponse"); |
There was a problem hiding this comment.
Create a constant for Envelope.Body.bulkResponse and use that instead. As it's also used in the MCOLJudgment and IssueWarrant classes you may want to create it as a public constant on a new parent class and have the three classes inherit from it.
|
|
||
| assertThat(xmlResponseCode, equalTo("Ok")); | ||
| Assertions.assertEquals(200, claimStatusSteps.getResponse().getStatusCode()); | ||
|
|
There was a problem hiding this comment.
Remove this blank line.
There was a problem hiding this comment.
Class needs to have a package line saying what package it is in.
Note that convention states that package names should all be in lower case. It would also make sense to create an "operations" package into which the get bulk feedback and submit bulk packages can be placed to group them logically. Taking this into account, and the comment about package names for the XMLFunctions class, I'd suggest the following package structure:
uk.gov.moj.sdt.operations
uk.gov.moj.sdt.operations.getbulkfeedback
uk.gov.moj.sdt.operations.submitbulk
The package line for this class would therefore be:
package uk.gov.moj.sdt.operations.submitbulk;
| XmlPath xmlPath = new XmlPath(claimStatusSteps.getCleanResponse()).setRoot("Envelope.Body.bulkResponse"); | ||
| String sdtBulkReference = xmlPath.getString("sdtBulkReference"); | ||
|
|
||
| assertThat(sdtBulkReference, matchesRegex("MCOL-\\d+-\\d+")); |
There was a problem hiding this comment.
The regular expression can be changed to MCOL-\\d{14}-\\d{9} (i.e. the second part should be exactly 14 digits and the third part exactly 9 digits) to more accurately reflect the format - see SdtBulkReferenceGenerator.getSdtBulkReference.
There was a problem hiding this comment.
The following previously made comments apply to this class:
- Reorganise class into the uk.gov.moj.sdt.operations.submitbulk package
- Add the package line
- Replace the JUnit4 imports with the JUnit5 equivalents and change code accordingly
- Organise the imports
|
|
||
|
|
There was a problem hiding this comment.
Remove one of these blank lines.
| System.out.println("xmlResponseCode: " + xmlResponseCode); | ||
| System.out.println("Customer Reference: " + customerReference); | ||
| System.out.println("SDT Bulk Reference: " + sdtBulkReference); | ||
| System.out.println("Submitted Date: " + submittedDate); | ||
| System.out.println("SDT Service: " + sdtService); | ||
| System.out.println("Request Count: " + requestCount); |
There was a problem hiding this comment.
Remove these System.out lines - they will be flagged by Sonar as an issue. If they really need to be output then consider the use of a logger (for an example see SubmitBulkForSystemTest class lines 48 and 81).
If there's no need to output the values then the variables on lines 85 to 89 can also be removed, unless the test is going to be changed to check the values of them.
| String custId = "10000002"; | ||
| String requestCount = "1"; | ||
| private String custIDInvalidErrorCode = "CUST_ID_INVALID"; | ||
| private String sdtBulkReferenceRegex = "MCOL-\\d+-\\d+"; |
There was a problem hiding this comment.
Regular expression can be changed to MCOL-\\d{14}-\\d{9}.
| custId = "12345678"; | ||
| setUp(); | ||
|
|
||
| XmlPath xmlPath = new XmlPath(XMLFunctions.getCleanResponse(response)).setRoot("Envelope.Body.bulkResponse"); |
There was a problem hiding this comment.
Changing the custId class field value like this will cause problems with subsequent tests. The custId value is never set back to the correct value afterwards, so the tests that run after will always fail with an invalid customer id error.
To prevent potential errors it might be an idea to make the following changes:
1.) Add randomNumber, custId and requestCount parameters to the soapRequestBody() method.
2.) Instead of calling setUp() here, call the soapRequestBody() method with the new customer id value, followed by soapResponse().
The advantage with this is that you won't need to remember to reset the custId field at the end of the test. It also guards against the case when the test fails due to an exception before the custId field can be reset.
| requestCount = "2"; | ||
| setUp(); | ||
|
|
||
| XmlPath xmlPath = new XmlPath(XMLFunctions.getCleanResponse(response)).setRoot("Envelope.Body.bulkResponse"); |
There was a problem hiding this comment.
Same comment applies here as for the validateCustID() test. The same suggested solution will work here too.
| // @Test | ||
| // public void issueWarrantResponseDescription() { | ||
|
|
||
| // checkStatusCode(response); | ||
|
|
||
| // String xmlResponse = response.asString(); | ||
| // xmlResponse = removeNamespaces(xmlResponse); | ||
|
|
||
| // XmlPath xmlPath = new XmlPath(xmlResponse).setRoot("Envelope.Body.bulkResponse.status.error"); | ||
| // String errorDescription = xmlPath.getString("description"); | ||
|
|
||
| // assertThat(errorDescription, containsString("Duplicate User File Reference 91 supplied")); | ||
| // } |
There was a problem hiding this comment.
Is this needed? If the test is valid but can't yet be run because of some other issue then uncomment it and annotate it with the JUnit @Disabled annotation. It's also recommended to add a comment to the annotation explaining why it's disabled, e.g.
@Disabled("Disabled until warrant functionality fixed")
If the test is not needed then delete it.
| System.out.println("Request Count: " + requestCount); | ||
|
|
||
| assertThat(xmlResponseCode, equalTo("Ok")); | ||
| Assertions.assertEquals(200, statusCode); |
There was a problem hiding this comment.
Could the checkStatusCode() method be used here instead of assertEquals()?
There was a problem hiding this comment.
The following previously made comments apply to this class:
- Reorganise class into the uk.gov.moj.sdt.operations.submitbulk package
- Add the package line
- Replace the JUnit4 imports with the JUnit5 equivalents and change code accordingly
- Organise the imports
|
|
||
|
|
There was a problem hiding this comment.
Remove one of these blank lines.
| e.printStackTrace(); | ||
| } | ||
|
|
||
| randomNumber = (int)(Math.random() * 1000000); // Generate a random number |
There was a problem hiding this comment.
Replace this with a call to XMLFunctions.generateRandomNumber()
| public String removeNamespaces(String xmlResponse) { | ||
| return xmlResponse.replaceAll("xmlns.*?(\"|\').*?(\"|\')", "") | ||
| .replaceAll("(<)(\\w+:)(.*?>)", "$1$3") | ||
| .replaceAll("(</)(\\w+:)(.*?>)", "$1$3"); | ||
| } |
There was a problem hiding this comment.
Remove this and use XMLFunctions.removeNamespaces() instead.
| public String getCleanResponse() { | ||
| String xmlResponse = response.asString(); | ||
| xmlResponse = removeNamespaces(xmlResponse); | ||
| return xmlResponse; | ||
| } |
There was a problem hiding this comment.
Remove this and use XMLFunctions.getCleanResponse() instead.
| System.out.println("xmlResponseCode: " + xmlResponseCode); | ||
| System.out.println("Customer Reference: " + customerReference); | ||
| System.out.println("SDT Bulk Reference: " + sdtBulkReference); | ||
| System.out.println("Submitted Date: " + submittedDate); | ||
| System.out.println("SDT Service: " + sdtService); | ||
| System.out.println("Request Count: " + requestCount); |
There was a problem hiding this comment.
Remove these System.out lines - they will be flagged by Sonar as an issue. If they really need to be output then consider the use of a logger (for an example see SubmitBulkForSystemTest class lines 48 and 81).
If there's no need to output the values then the variables on lines 87 to 91 can also be removed, unless the test is going to be changed to check the values of them.
| XmlPath xmlPath = new XmlPath(getCleanResponse()).setRoot("Envelope.Body.bulkResponse"); | ||
| String sdtBulkReference = xmlPath.getString("sdtBulkReference"); | ||
|
|
||
| assertThat(sdtBulkReference, matchesRegex("MCOL-\\d+-\\d+")); |
There was a problem hiding this comment.
Regular expression can be changed to MCOL-\\d{14}-\\d{9}.
There was a problem hiding this comment.
All the test methods in this class have "Warrant" in the name. They should be changed to "Judgment" instead (probably a copy/paste issue).
| // @Test | ||
| // public void issueWarrantResponseDescription() { | ||
|
|
||
| // checkStatusCode(response); | ||
|
|
||
| // String xmlResponse = response.asString(); | ||
| // xmlResponse = removeNamespaces(xmlResponse); | ||
|
|
||
| // XmlPath xmlPath = new XmlPath(xmlResponse).setRoot("Envelope.Body.bulkResponse.status.error"); | ||
| // String errorDescription = xmlPath.getString("description"); | ||
|
|
||
| // assertThat(errorDescription, containsString("Duplicate User File Reference 91 supplied")); | ||
| // } |
There was a problem hiding this comment.
Is this needed? If the test is valid but can't yet be run because of some other issue then uncomment it and annotate it with the JUnit @Disabled annotation. It's also recommended to add a comment to the annotation explaining why it's disabled.
If the test is not needed then delete it.
| System.out.println("Request Count: " + requestCount); | ||
|
|
||
| assertThat(xmlResponseCode, equalTo("Ok")); | ||
| Assertions.assertEquals(200, statusCode); |
There was a problem hiding this comment.
Could this use checkStatusCode() instead?
There was a problem hiding this comment.
Should this be empty and is it needed? No tests reference this file.
There was a problem hiding this comment.
Is this file needed? There don't appear to be any tests that use it.
| <!-- Defendant id must be 1 or 2 --> | ||
| <brthsp:defendantId>1</brthsp:defendantId> | ||
| <!-- Breathing space notification type must be BS, BC, MH or MC --> | ||
| <brthsp:breathingSpaceNotificationType>BT</brthsp:breathingSpaceNotificationType> |
There was a problem hiding this comment.
If this file is needed, note that the breathing space type here is an invalid one. The valid types are given in the comment above.
There was a problem hiding this comment.
Is this file needed? There don't appear to be any tests that use it.
There was a problem hiding this comment.
Is this file needed? There don't appear to be any tests that use it.
If the file is needed then the name should be "combinedJudgmentWarrant".
There was a problem hiding this comment.
If the validGetBulkFeedback class isn't reinstated then it doesn't appear that this file is needed.
There was a problem hiding this comment.
Is this file needed? There don't appear to be any tests that use it.
There was a problem hiding this comment.
Is this file needed? There don't appear to be any tests that use it.
There was a problem hiding this comment.
Is this file needed? There don't appear to be any tests that use it.
| group = 'uk.gov.hmcts.civil.sdt' | ||
| version = '0.0.1' | ||
| sourceCompatibility = '11' | ||
| targetCompatibility = '11' |
There was a problem hiding this comment.
why we are using 11 only, can we use 17
| implementation 'org.apache.xmlbeans:xmlbeans:3.1.0' | ||
| testImplementation 'net.serenity-bdd:serenity-core:3.2.0' | ||
| testImplementation 'net.serenity-bdd:serenity-junit:3.2.0' | ||
| testImplementation 'io.rest-assured:rest-assured:4.4.0' |
There was a problem hiding this comment.
can we use 5.3.1 version as this is latest
| @@ -221,6 +229,14 @@ allprojects { | |||
| apply plugin: 'java' | |||
|
|
|||
| dependencies { | |||
| implementation 'org.apache.xmlbeans:xmlbeans:3.1.0' | |||
| testImplementation 'net.serenity-bdd:serenity-core:3.2.0' | |||
| testImplementation 'net.serenity-bdd:serenity-junit:3.2.0' | |||
There was a problem hiding this comment.
can we use 3.8.1 version as this is latest for serenity-core and serenity-junit
| @@ -221,6 +229,14 @@ allprojects { | |||
| apply plugin: 'java' | |||
|
|
|||
| dependencies { | |||
| implementation 'org.apache.xmlbeans:xmlbeans:3.1.0' | |||
There was a problem hiding this comment.
can we use 5.1.1 version as this is latest for xmlbeans
| testImplementation 'io.rest-assured:rest-assured:4.4.0' | ||
| implementation 'io.rest-assured:json-path:4.4.0' | ||
| testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.2' | ||
| testImplementation 'org.junit.jupiter:junit-jupiter:5.7.2' |
There was a problem hiding this comment.
check these as well for the latest versions
| @@ -0,0 +1,53 @@ | |||
| import io.restassured.RestAssured; | |||
| import io.restassured.response.Response; | |||
| import net.serenitybdd.junit.runners.SerenityRunner; | |||
There was a problem hiding this comment.
why the name of the file ends with .txt
Before creating a pull request make sure that:
Please remove this line and everything above and fill the following sections:
JIRA link (if applicable)
Change description
Does this PR introduce a breaking change? (check one with "x")