[PHEE-684] Reporting CRUD APIs Integration Test #296
Conversation
| @@ -210,7 +210,7 @@ jobs: | |||
| name: Test execution | |||
| no_output_timeout: 30m | |||
| command: | | |||
| ngrok config add-authtoken $AUTH_TOKEN | |||
| ngrok config add-authtoken 2i8JgHJji5KPQvUpByBDrQ6pfRl_Je6B9jDwHDUadvvvZvGe | |||
There was a problem hiding this comment.
Need to be careful while raising PRs to not push changes done for local testing.
There was a problem hiding this comment.
Actually there is only one ngnox in openci so if someone else run there pipeline mine will not work that's why used this
There was a problem hiding this comment.
What are ngnox and openci, never heard of them.
There was a problem hiding this comment.
Can this be merged? If not, and you made the change for testing purposes, then the PR is not in the ready for review (RPR) state. It should have been a draft PR and moved to ready for review only after reversing this change.
|
|
||
| @Configuration | ||
| @EnableConfigurationProperties | ||
| @ConfigurationProperties(prefix = "tenantconfig") | ||
| @Component |
There was a problem hiding this comment.
We don't need Component annotation here.
| @@ -17,6 +17,10 @@ operations-app: | |||
| variables: "/api/v1/variables" | |||
| transactionRequests: "/api/v1/transactionRequests" | |||
| actuator: "/actuator/health" | |||
| reportEndpoint: "/reports" | |||
| reportCreate : "/reports" | |||
There was a problem hiding this comment.
Why multiple configurations for the same values? 1 or 2 configurations would have sufficed here instead of 4.
| @@ -36,16 +36,16 @@ public class ScenarioScopeState { | |||
| protected String fspId; | |||
|
|
|||
| protected String batchId; | |||
| protected String tenant; | |||
| protected String response; | |||
| public String tenant; | |||
There was a problem hiding this comment.
Why are these variables made public?
| protected String request; | ||
|
|
||
| protected String payeeIdentity; | ||
| protected String requestId; | ||
| protected String callbackBody; | ||
|
|
||
| protected Integer statusCode; | ||
| protected String accessToken; | ||
| public String accessToken; |
| And the response should contain a unique report ID | ||
|
|
||
| When I call the get list of reports API with expected status of 200 | ||
| Then the response should contain a list of reports |
There was a problem hiding this comment.
These statements do not explain why are you doing Get reports multiple times, what are you checking here?
| try { | ||
| scenarioScopeState.createReportBody = objectMapper.writeValueAsString(reportDTO); | ||
| } catch (JsonProcessingException e) { | ||
| logger.error("Unable to convert the DTO : {}", e); |
There was a problem hiding this comment.
The test should fail here as the request body is not created. What are you sending in your API request by continuing the execution?
|
|
||
| MatcherAssert.assertThat(jsonResponse.get("reportName").asText(), Matchers.equalTo("Sample Report")); | ||
| MatcherAssert.assertThat(jsonResponse.get("reportType").asText(), Matchers.equalTo("Financial")); | ||
| MatcherAssert.assertThat(jsonResponse.get("reportSubType").asText(), Matchers.equalTo("Monthly")); |
There was a problem hiding this comment.
Instead of using the MatcherAssert class repeatedly, use its functions directly.
| reportDTO.setReportName(null); | ||
| reportDTO.setReportType("InvalidType"); | ||
| reportDTO.setReportSubType("InvalidSubType"); | ||
| reportDTO.setReportCategory("Operational"); |
There was a problem hiding this comment.
How are these values other than report Name Invalid, what checks would they fail? There are no checks for these fields in your API.
|
Microcommits are not making any sense. The same message repeated multiple times, instead of fixing one check style issue in one micro-commit, all such fixes can be done in one. If the fixes are big enough then specify the fix in the commit message. Otherwise, squash your commits. |
PHEE-684 Reporting CRUD APIs
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Followed the PR title naming convention mentioned above.
Acknowledge that we will not merge PRs that are not passing the checks ("green") - it is your (author's) responsibility to get a proposed PR to pass all the checks, not primarily the project's maintainers.
The PR title should include a JIRA ticket
Design-related bullet points or design document links related to this PR added in the description above.
Updated corresponding Postman Collection or API documentation for the changes in this PR.
Create/update unit or integration tests for verifying the changes made.
Add required Swagger annotation and update API documentation with details of any API changes if applicable
Followed the naming conventions as given in https://docs.google.com/document/d/1Q4vaMSzrTxxh9TS0RILuNkSkYCxotuYk1Xe0CMIkkCU/edit?usp=sharing
Followed coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
FYI our guidelines for code reviews same as https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.