-
Notifications
You must be signed in to change notification settings - Fork 2
Migrate Shortcircuit tests to wiremock #927
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
base: main
Are you sure you want to change the base?
Conversation
| public void verifyParametersDefault() { | ||
| WireMockUtilsCriteria.verifyPostRequest(wireMock, "/v1/parameters/default", Map.of()); | ||
| } |
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 think we should use the method that already exists (with the param nbRequests at 1) so we don't end up with twice the number of function needed.
|
|
||
| public void stubGetResultCsv(String resultUuid, byte[] csvContent) { | ||
| wireMock.stubFor(WireMock.post(urlPathEqualTo("/v1/results/" + resultUuid + "/csv")) | ||
| .willReturn(aResponse() |
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.
why do we switch from WireMock.ok() to aResponse().withStatus(200) ?
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.
ok is more readable I agree
| } | ||
|
|
||
| public void stubParametersDefault(String statusJson) { | ||
| public void stubPostParametersDefault(String responseBody) { |
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.
we already have it
study-server/src/test/java/org/gridsuite/study/server/utils/wiremock/WireMockStubs.java
Lines 751 to 754 in 89f4761
| public UUID stubParametersDefault(String responseBody) { | |
| return wireMock.stubFor(WireMock.post(WireMock.urlPathEqualTo("/v1/parameters/default")) | |
| .willReturn(WireMock.ok().withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE).withBody(responseBody))).getId(); | |
| } |
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.
we already have it
study-server/src/test/java/org/gridsuite/study/server/utils/wiremock/WireMockStubs.java
Lines 751 to 754 in 89f4761
public UUID stubParametersDefault(String responseBody) { return wireMock.stubFor(WireMock.post(WireMock.urlPathEqualTo("/v1/parameters/default")) .willReturn(WireMock.ok().withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE).withBody(responseBody))).getId(); }
I don't think we should use the one from WireMockStubs, the goal is to remove all the methods that shoudn't be here in the first place so we need to put these functions in the appropriate Stubs class, which is what he did here. What we can do is remove this function from WireMockStubs but It's used elsewhere and it's not in the scope of this PR to change that in the other tests
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.
Yes that's what I suggested here #927 (comment)
But OK for cleaning in an other TS.
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'll remove those in the deprecated class there's only one usage so it's fine
| public void verifyParameterPut(WireMockServer wireMockServer, String paramUuid) { | ||
| wireMockServer.verify( | ||
| putRequestedFor(urlEqualTo("/v1/parameters/" + paramUuid)) | ||
| putRequestedFor(urlEqualTo("/v1/parameters/" + paramUuid)) |
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.
Try to keep previous indentation to preserve the Git history
| WireMockUtilsCriteria.verifyPostRequest(wireMock, "/v1/parameters/default", Map.of(), nbRequests); | ||
| } | ||
|
|
||
| public void stubDeleteParameters(String parametersUuid) { |
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 think we should either delete parameters stubs from this class or from WireMockStubs but we shouldnt duplicate them
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 can delete the ones from WireMockStubs they don't seem widely used
| null); | ||
| } | ||
|
|
||
| public void stubParameterPut(String paramUuid, String responseJson) { |
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.
Why in SC stubs file ?
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.
Moved those to computation stubs and deprecated pre existing faulty ones as discussed
|



PR Summary