-
Notifications
You must be signed in to change notification settings - Fork 472
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
docs: partial reports can be submitted #3179
docs: partial reports can be submitted #3179
Conversation
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
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.
After thinking it over, I do agree that it's better to get partial reports rather than no reports because this will increase our awareness of implementations in the field and enable more opportunities to engage with them.
I do think we may need to adjust the display to accommodate (make sure it's clear, and perhaps delineated when an implementation is reporting partial). I approve,
/approve
However I would like us to make sure we have a follow-up issue in place for considering how the display should look like for this (I don't consider it a blocker though) before we merge:
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mlavacca, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Yes, it makes sense. I've opened #3183 to capture this. Let me know if it fits with what you had in mind :) |
LGTM /unhold |
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.
Thanks @mlavacca! LGTM, just a couple nits where further clarification could be useful.
that all the core conformance tests have been successfully run as well as all | ||
the tests related to the supported extended features. No reports with partial | ||
or failing results can be accepted. | ||
need to have the `result` fields (core and extended) set to `success` or `partial`. |
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 seems like we probably need to include some guidelines for how we handle partial
conformance results. Maybe it's sufficient to just say that they'll be displayed separately?
conformance/reports/README.md
Outdated
It means that all the core conformance tests have been successfully run or properly | ||
skipped as well as all the tests related to the supported extended features. No | ||
reports with failing results can be accepted. |
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 there's another reason for partial conformance - the tests may all pass, but the reproduction required some kind of additional unexpected steps.
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 reworded this section to include this information
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
All of @robscott's feedback is addressed, going to get this one in. /lgtm |
What type of PR is this?
/kind documentation
/area conformance
What this PR does / why we need it:
After discussing the possibility of accepting partial reports in #3021, we eventually decided to encourage implementations to start submitting reports even if there is no full conformance. This PR updates the documentation to reflect this final decision.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: