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
Display deployment status for cf app command [v8] #3041
Display deployment status for cf app command [v8] #3041
Changes from 2 commits
8749bf3
735d66f
a555cf7
808089f
d721cc7
e3fd36a
9ced2f3
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.
QUESTION
Are all of these warnings relevant to this scenario? If not, maybe the
ContainElement
matcher is more appropriate — otherwise this assertion will need to be updated any time any of the other, unrelated warnings are changed.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, it is because of when the test is happening (after all other function stubs are invoked) as we saw earlier.
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 get that these warnings are all returned in this scenario. But IMHO they're not all relevant to this scenario, and therefore should probably not be enumerated.
If some logic changes for one of the other warnings we'd need to rewrite this assertion to match. The test becomes more robust — and more expressive — by limiting the assertion to checking for just the presence of the single, relevant warning and not the exhaustive contents of the full array.
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 agree. We discussed this while working on these tests and moved it around to suit the process when we were pairing. If something changes for these warning, most of the tests in this will need to 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.
SUGGESTION
These comments don't really add much information on top of the names. Just delete 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.
I'm ok with it. Just placed them for consistency sake.
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.
Would actually suggest deleting all the other ones, then. None of them add any information beyond what the identifier itself does.
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.
QUESTION
Same as above:
Deployment
? What does that actually look like when it comes over the wire?Deployment
DTO?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 response as above. #3041 (comment)