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
Adding MIOpen Contributing Guide #932
Adding MIOpen Contributing Guide #932
Changes from 2 commits
bef683a
ee1863d
bd7bed1
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 want to clarify how we understand "passed necessary CI".
Is it only about tests in current CI which should be passed.
Or also to check that the coverage is sufficient and to ask the author to add tests to the CI, if necessary.
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.
@shurale-nkn "add tests to the CI" is indicated in Point 5 below, so passing CI here meant tests in current CI which should be passed only.
However, it's a good point about sufficient coverage, and we may have different set of CI for different types of PRs. How would someone new know about which set of CI needs to be passed? :(
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.
Right now we have a "full set" of tests. If "full set" is passed, then it is Ok.
It is also possible to manually narrow or extend the set or tests. This is also Ok provided that all the reviewers and the author agree on 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.
It may worth explaining this in contributing rules.
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.
Mostly my question is about unit tests and verification.
And 'full test' can't guarantee any of this.
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.
Maybe it is the contributor's job to always provide a subsection in the PR description about which subset of test he used for functionality regression. If this is generic changes, then it could be one of the
full test
stages. If else, a specific test stage need to be specified that can cover the changes.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.
@shurale-nkn
If so, then the "full test" has an issue and needs to be updated or fixed.
[Off-topic] BTW this is what you are doing right now with that "reshuffle and reduce test cases", right?
@junliume Yes. If the existing test set does not cover the PR, then the contributor and reviewers should take care of it. Ideally, add necessary tests in the same PR.
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.
[Off-topic]
No, it's about our tests do testing only for fastest solution, not for all, so we can't guaranty correctness of all solvers.
I asked to add this feature long time ago. It should be written somewhere in the plans.