Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[CONTINT-685] - initial tests for validation of OpenApi request #767
base: master
Are you sure you want to change the base?
[CONTINT-685] - initial tests for validation of OpenApi request #767
Changes from 10 commits
b2e9234
cd4e3ec
19e5d98
3964e56
529f282
ab08fc3
6afb106
366b993
0352957
182a8f6
57254c4
f174dee
913274f
1b1a8ad
576e8eb
83abcfa
5dc73c5
49c27b5
64904cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not sure if we should go this path - I'm not in favor of adding another static (json or yaml) requests - I think we should introduce some Java DTOs and use them. With introduction of these DTOs and new Mappers we could get rid of
UpdateNodeEventSerializer
,DeleteNodeEventSerializer
and some static requests from our tests - WDYT @tpage-alfresco and @Pawel-608 ?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.
The idea here is just to extract the static json from e.g.
CreateRequestIntegrationTest
and put it in a file where two tests can access it. I think what you're suggesting is to combine the tests so that we validate the output from the Connector directly against the remote schema, but I'm concerned that if the test starts failing then we won't know if it's due to our code changes or changes in the remote schema.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.
Additionally I think we want to validate the exact output of the Connector (rather than just its validity), because otherwise we would pass tests by e.g. only producing delete requests.
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, I get your point - the idea is to perform validation in two steps:
One downside what I see here is: maintain complexity - if someone will forget to update this static yaml request we might end up in situation where Connector will be sending not valid requests and we might notice it quite late
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 - I agree. It's important that all static requests are also included in e2e tests so that we know they can be produced by the application. I can't think of a neat way to verify that.
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 made a suggestion of how we could ensure we test all requests in the folder against the schema here: #767 (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.
I think, that we could leave
CreateRequestIntegrationTest
and othersusecase.e2e
tests as they are and introduce new test verifying if our code complies with OpenAPI spec. If only this new one test would fail, it would mean, that the spec has changed. While when more tests will start to fail, then someone messed something up in the code.Our Endpoint-to-Endpoint tests are very handy and bring a lot of value presenting in a simple way what will be the outcome for different repo events, so we shouldn't change 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.
For this method of testing to be valid we need to ensure that we're also checking the application can output these requests. We are doing this for
create-document-request.yml
inCreateRequestIntegrationTest.java
, but I don't think we're doing it for the other three yml files.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've updated testUpdateRequest() in UpdateRequestIntegrationTest.java class with "/expected-hxinsight-requests/update-document-request.yml". In UpdateRequestIntegrationTest.java there are test cases which use different request body - we do not use those in OpenApi tests. DeleteRequestIntegrationTest.java class was updated with "/expected-hxinsight-requests/delete-document-request.yml". There is no body for PresignedUrls case so no need for update there.