-
Notifications
You must be signed in to change notification settings - Fork 0
Ragnarok 148 #1
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?
Ragnarok 148 #1
Conversation
| public class HttpRequest { | ||
| private Response response; | ||
|
|
||
| public Response getResponse() { |
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.
Looks good and this definitely works! In the future, you could consider using Lombok and its setter and getter notations that would help reduce the code you write especially if you have multiple class variables.
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.
Thanks @zyrondias - we learnt about lombok yesterday, though we wanted to keep the old style for visualisation purposes. :)
| @@ -0,0 +1,6 @@ | |||
| Feature: testing that private endpoint can be reached | |||
| Scenario: client calls GET /private/status | |||
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.
You could consider adding a bit more information to the scenario - For example, is it a happy case or a sad case? It helps make it explicit when looking for which particular test has failed
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.
Noted @zyrondias - makes sense. @rosa03 and I will look to add.
| private PrivateController privateController; | ||
| @Mock | ||
| HeadersService headersService; | ||
|
|
||
| @BeforeEach | ||
| public void setUp(){ | ||
| privateController = new PrivateController(headersService); | ||
| } |
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.
Have a look into @Injectmocks annotation. Its quite a handy little annotation and will save you from writing lines of code to initialise the object and pass in a mock
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.
To be honest we used this @Injectmocks yesterday but failed to push it up to the repo. @rosa03
tasnimb
left a comment
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.
Looks good 👍
| // //@ValueSource(ints = {HttpStatus.BAD_REQUEST}) | ||
| // private static List<HttpStatus> getBadHTTPStatuses(){ | ||
| // return List.of(HttpStatus.BAD_REQUEST, HttpStatus.BAD_GATEWAY); | ||
| // } | ||
| // @ParameterizedTest | ||
| // @MethodSource("getBadHTTPStatuses") | ||
| // public void statusCodeIsIncorrect(HttpStatus httpStatus){ | ||
| // PrivateController privateController = new PrivateController();; | ||
| // | ||
| // assertEquals(httpStatus.value(), privateController.status().getStatusCode().value()); | ||
| // | ||
| // System.out.println(httpStatus); | ||
|
|
||
| // assertTrue(privateController.status().getStatusCode().); |
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.
If these tests are not being used then maybe get rid of 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.
Thanks - we kept it as an example of another way of testing using parameterized tests. @rosa03
zyrondias
left a comment
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.
Looks good just a few comments and suggestions
Linked to Ticket - Ragnarok-148
Implement a private/status endpoint to return response body "OK"
Return a status code 200
Set up Cucumber for functional testing
Add a x-sky-request-id to the response endpoint headers