-
Notifications
You must be signed in to change notification settings - Fork 29
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
Common artifacts for testing error scenarios for device and phoneNumber #325
Common artifacts for testing error scenarios for device and phoneNumber #325
Conversation
Common error scenarios for POST operations with device as input either in the request | ||
body or implied from the access. | ||
|
||
Artifact parameters (to be replaced by values according to the API operation): | ||
|
||
- {{feature_identifier}} | ||
|
||
This is not a complete feature but a collection of scenarios that can be applied | ||
with minor modifications to test plans. | ||
|
||
These scenarios assume that other properties not explicitly mentioned in the scenario | ||
are set by default to a valid value. This can be specified in the feature Background. | ||
|
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 can have more clearer feature description to let know what should be removed or replaced when using the templates. Mark the block to be removed when instantiating this feature file. Eg. $Hint_Start --to be removed { } $Hint_End .
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.
All of this text has not to be copied, as it says "This is not a complete feature but a collection of scenarios that can be applied with minor modifications to test plans.". I formatted it as a Feature so Gherkin format is recognized by the IDE. If it is not understood in this way I may clarify it better.
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.
@jlurien Please clarify in more better way
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.
@mdomale, please review the new wording. Feel free to add any suggestions
|
||
# Error scenarios for management of input parameter device | ||
|
||
# If the access token identifies a device, error 422 UNNECESSARY_DEVICE may be returned instead |
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.
Scenario details are not clear,if we expect 422 then we anticipate 422 being considered and written in below mentioned scenario where we see only 400.
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 have removed the misleading comment, here it should be 400
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.
LGTM
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.
LGTM
@jlurien except above minor comments PR LGTM |
@mdomale @PedroDiez @patrice-conil hi, you may need to approve again after the last commit to address remaining 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.
Minor editorial
LGTM in advance
Co-authored-by: Pedro Díez García <[email protected]>
Co-authored-by: Pedro Díez García <[email protected]>
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.
LGTM
LGTM |
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.
LGTM
Hi @rartych, al past reviewers have given the LGTM. Please, double check if it is ready to be merged now. Thanks |
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.
LGTM
What type of PR is this?
What this PR does / why we need it:
Adds the first common artifacts, with scenarios that can be included in test plans for operations that receive an input with device and phoneNumber. They must be aligned with the guidelines in Commonalities and indicate a proper way to validate that guidelines are enforced by the implementation.
Each artifact is assigned a code number, e.g.
C01
, and within each artifact, each scenario is numbered with the artifact code "." scenario number, e.g.C01.01
, plus a short description.Which issue(s) this PR fixes:
Fixes #323
Does this PR introduce a breaking change?
Special notes for reviewers:
device
andphoneNumber
, and that breaks compatibility with the error codes used in past meta release Fall24.422 UNNECESSARY_IDENTIFIER
, which deprecated old use case for403 INVALID_TOKEN_CONTEXT
. as proposed in PR Update error codes and info.description template for device / phone number identifiers #324Changelog input