-
Notifications
You must be signed in to change notification settings - Fork 0
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-3175 Allow testers to specify new or updated client registration attempt #17
Merged
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
14091a2
Create client_registration_status input in test, groups
alisawallace 0e0e605
Update registration success test to check input for expected response…
alisawallace 83a1255
Update registration success spec tests to check for 200 vs 201 respon…
alisawallace 3297c87
Update input instructions and test descriptions
alisawallace 486e4f1
Accept 200 or 201 response status for successful updated registration…
alisawallace 8505a06
Update input instructions and test descriptions
alisawallace ddf2146
Merge branch 'main' into FI-3175-allow-registration-update
alisawallace File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
This is considerably more strict than the spec, which just says:
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 felt more comfortable with this strict behavior because this is really more of a convenience option for testers that they can opt in to use, rather than a behavior the tests are requiring them to demonstrate. The only thing they are technically required to demonstrate is that a new registration returns a 201 response, it's just that in practice this can be difficult to do (especially in repeated test runs) since we're always working with a limited set of client identities that are likely to already be in the system.
However, we could expand the acceptable range of valid response status codes for the "update" case to make it a little more flexible (accept either 201 or 200? Anything in the 200 range?) What I don't want to do is open it up too much to where the test can technically pass when the modification attempt didn't actually succeed. The next test (registration success contents) would likely catch that but still.
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 201 or 200 for an update would be fine.
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.
Alright, I updated the code, unit tests, and descriptions to reflect this.