-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Document tests in the run-make
directory (A to C)
#124847
Conversation
This comment has been minimized.
This comment has been minimized.
run-make
directoryrun-make
directory (A to C)
Some changes occurred in run-make tests. cc @jieyouxu |
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 for adding some docs. I think test docs should have (this is mostly a remark):
- The why: why does this test exist? relevant issues, PRs and discussions?
- The what: what is this test trying to test?
- The how: how does this test test what it wants to test? this should only be elaborated upon if the how is not trivial or not immediately obvious.
If possible, tests (including its docs) should be somewhat self-contained: it's okay to link to relevant issues and PRs for further context, but it should be possible to understand (at a basic level) why a test exists and what it's trying to test without having to go to the relevant links.
I left some remarks for the test comments.
Also it might make more sense to update docs alongside ports, because there might be subtleties that is not easy to realize when just reading the Makefiles. |
All review feedback has been implemented.
That is what I was realizing as well... but I was wondering if these comments are better than what there currently is, which is "nothing". The URLs might be a "nice-to-have" instead of digging through the commit history, which spans over a decade and implies file renames and moves. If you think it is better to wait, this PR can be closed. I'll keep these notes in my personal fork, though, as I've found them useful as reference for the porting tasks. |
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.
One nit then r=me
@bors delegate+ |
✌️ @Oneirical, you can now approve this pull request! If @jieyouxu told you to " |
r=me after CI is green |
☀️ Test successful - checks-actions |
Finished benchmarking commit (8f9080d): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 673.66s -> 673.028s (-0.09%) |
Part of the #121876 project.
This PR adds comments to some
run-make
tests which lack one, explaining what is being tested. If possible, a link to the relevant PR or Issue responsible for the test is also provided.This will help the porting efforts to
rmake.rs
, and will also allow maintainers to focus efforts on tests which are more pertinent to port. For example, this test will become useless after all tests containingCGREP
are successfully ported.In order to simplify review and at the suggestion of Kobzol on the rust-lang #gsoc Zulip, only the first 23 comments are part of this PR. If it is merged, future PRs will ensue commenting the rest of the tests.
Could be an UI test:
dep-info-doesnt-run-much