Skip to content
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

Merged
merged 3 commits into from
May 9, 2024

Conversation

Oneirical
Copy link
Contributor

@Oneirical Oneirical commented May 7, 2024

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 containing CGREP 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

@rustbot
Copy link
Collaborator

rustbot commented May 7, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2024
@rust-log-analyzer

This comment has been minimized.

@Oneirical Oneirical changed the title Document tests in the run-make directory Document tests in the run-make directory (A to C) May 8, 2024
@Oneirical Oneirical marked this pull request as ready for review May 8, 2024 02:17
@rustbot
Copy link
Collaborator

rustbot commented May 8, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

Copy link
Contributor

@jieyouxu jieyouxu left a 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.

tests/run-make/alloc-no-oom-handling/Makefile Outdated Show resolved Hide resolved
tests/run-make/alloc-no-rc/Makefile Outdated Show resolved Hide resolved
tests/run-make/alloc-no-sync/Makefile Outdated Show resolved Hide resolved
tests/run-make/allocator-shim-circular-deps/Makefile Outdated Show resolved Hide resolved
tests/run-make/archive-duplicate-names/Makefile Outdated Show resolved Hide resolved
tests/run-make/c-dynamic-dylib/Makefile Outdated Show resolved Hide resolved
tests/run-make/codegen-options-parsing/Makefile Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Contributor

jieyouxu commented May 8, 2024

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.

@Oneirical
Copy link
Contributor Author

Oneirical commented May 9, 2024

All review feedback has been implemented.

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.

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.

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.

Copy link
Contributor

@jieyouxu jieyouxu left a 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

tests/run-make/cat-and-grep-sanity-check/Makefile Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Contributor

jieyouxu commented May 9, 2024

@bors delegate+

@bors
Copy link
Contributor

bors commented May 9, 2024

✌️ @Oneirical, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@jieyouxu
Copy link
Contributor

jieyouxu commented May 9, 2024

r=me after CI is green

@Oneirical
Copy link
Contributor Author

@bors r=@jieyouxu

@bors
Copy link
Contributor

bors commented May 9, 2024

📌 Commit bdab8b1 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2024
@bors
Copy link
Contributor

bors commented May 9, 2024

⌛ Testing commit bdab8b1 with merge 8f9080d...

@bors
Copy link
Contributor

bors commented May 9, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 8f9080d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 9, 2024
@bors bors merged commit 8f9080d into rust-lang:master May 9, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 9, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8f9080d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This 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.

Cycles

Results

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.66s -> 673.028s (-0.09%)
Artifact size: 315.97 MiB -> 315.78 MiB (-0.06%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants