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
FI-3518/FI-3519: Add basic Bulk Data client test suite, groups, and tests #36
base: main
Are you sure you want to change the base?
FI-3518/FI-3519: Add basic Bulk Data client test suite, groups, and tests #36
Changes from 1 commit
fda96b7
b532026
cd44d9f
c2b22c0
671c25a
ccb48cb
0f3b3f3
6221b78
22c8b87
5ff5dd7
1b5934a
d315446
512d865
3021198
3560f0f
b93dc39
5dc8866
f5adc33
aafd2fc
97b971f
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.
Not sure if this is something that should be changed, but I am thinking to be consistent with the rest of the bulk data test suites, there is no need to add the Client module here
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 want to separate out the client tests in some way, right? Do you mean move
Client
up one level e.g. have amodule BulkDataV200Client
?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 sorry that is what I meant, so rather than have a client module under the BulkDataV200 module, combine them so it is something like
module BulkDataV200Client
. Again not sure if this is a necessary change, I was just looking at the rest of the Bulk Data test kit and saw that the server tests do not have a server module.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.
Yeah that makes sense, I've made this change.
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.
Including the Tags module is not necessary here. I believe if you add
require_relative 'tags'
to the client suite file, you can use the tag constants in all of the endpoint 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'm not 100% sure what you mean. I'm bringing in the Tags module as a mixin to get access to the constants directly.
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.
Oops nevermind with the way the Tags and URLs files are set up, you are right! I was looking at other test kits where it is done differently where the tags file does not have a Tags module defined. So for example for this test kit, the tags file would look like this:
Again maybe not a necessary change, but it does allow you to use constants from the Tags and URLs files without needing to include a new module.
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.
Oh I see what you mean - it's definitely better to be consistent with the other test kits. I've made this change.
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.
Including ExportTypes I believe also is not necessary, since you are just using constants from that file like like the tags 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.
Same comment as above (about Tags)
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.
Same comment as above (about Tags) and how if a new ExportTypes module is not defined in the export types file, then you do not need to include the module and can instead use the constants directly in every file by requiring the export types file from the parent suite.
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.
Fixed.