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.

Sign up for GitHub

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

Jump to bottom

UI to unit test for those using Cell/RefCell/UnsafeCell #76454

Merged
merged 14 commits into from
Sep 28, 2020
Merged

UI to unit test for those using Cell/RefCell/UnsafeCell #76454

merged 14 commits into from
Sep 28, 2020

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Sep 7, 2020 •
edited
Loading

Helps with #76268.

I'm working on all files using Cell and moving them to unit tests when possible.

r? @matklad

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2020
@poliorcetics poliorcetics changed the title [WIP] UI to unit test WIP: UI to unit test Sep 7, 2020
@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 7, 2020
@poliorcetics
Copy link
Contributor Author

poliorcetics commented Sep 8, 2020 •
edited
Loading

@matklad If you have the time, you can start reviewing this right now. I'm doing a commit per deletion of a file + add its tests into the relevant file inside alloc|core|std.

I can mark commits that address comments with review: if you want.

If necessary, I'll squash when this is done (the PR as a whole).

@poliorcetics poliorcetics changed the title WIP: UI to unit test UI to unit test for those using Cell/RefCell/UnsafeCell Sep 13, 2020
@poliorcetics
Copy link
Contributor Author

I think I am done with the tests using (Ref|Unsafe)?Cell, this is not a WIP anymore.

@bors
Copy link
Contributor

bors commented Sep 21, 2020

☔ The latest upstream changes (presumably #77004) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

matklad
matklad reviewed Sep 24, 2020
View reviewed changes
Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM!

Sorry for a high review latency on this one -- I am just back from my vacation :)

src/test/ui/option-unwrap.rs
}

assert_eq!(x.get(), 0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test feels like it intends to stresses auto-generated drop impl, rather than any specific impl details of standard library, so I think it's OK to keep as an UI test. The best way to check if this intends to check compiler or stdlib is to look at the PR which introduced the test: e47d2f6#diff-518b4158ed084dab26591f1ec09dffef. In this case, this is a change to the compiler.

But keeping this as a #[test] sounds fine by me as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong feelings, I just felt it was simple enough to move. I can move it back if necessary.

library/core/tests/option.rs Outdated Show resolved Hide resolved
library/alloc/tests/fmt.rs Outdated Show resolved Hide resolved
library/alloc/tests/fmt.rs Show resolved Hide resolved
library/alloc/tests/boxed.rs Outdated Show resolved Hide resolved
library/core/tests/lib.rs Outdated Show resolved Hide resolved
library/core/tests/slice.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

Except for the discussion about src/test/ui/option-unwrap.rs, I addressed all your comments. Thanks for the review !

@matklad
Copy link
Member

matklad commented Sep 28, 2020

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Sep 28, 2020

📌 Commit a61b963 has been approved by matklad

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 28, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 28, 2020
@RalfJung
Rollup merge of rust-lang#76454 - poliorcetics:ui-to-unit-test-1, r=m…
f9dfbb4 
…atklad

UI to unit test for those using Cell/RefCell/UnsafeCell

Helps with rust-lang#76268.

I'm working on all files using `Cell` and moving them to unit tests when possible.

r? @matklad
@RalfJung RalfJung mentioned this pull request Sep 28, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 28, 2020
@Dylan-DPC
Rollup merge of rust-lang#76454 - poliorcetics:ui-to-unit-test-1, r=m…
4bb2369 
…atklad

UI to unit test for those using Cell/RefCell/UnsafeCell

Helps with rust-lang#76268.

I'm working on all files using `Cell` and moving them to unit tests when possible.

r? @matklad
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 28, 2020
Closed
@RalfJung RalfJung mentioned this pull request Sep 28, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2020
@bors
Auto merge of rust-lang#77302 - RalfJung:rollup-n8gg3v6, r=RalfJung
fc2daaa 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#76454 (UI to unit test for those using Cell/RefCell/UnsafeCell)
 - rust-lang#76474 (Add option to pass a custom codegen backend from a driver)
 - rust-lang#76711 (diag: improve closure/generic parameter mismatch)
 - rust-lang#77170 (Remove `#[rustc_allow_const_fn_ptr]` and add `#![feature(const_fn_fn_ptr_basics)]`)
 - rust-lang#77194 (Add doc alias for iterator fold)
 - rust-lang#77288 (fix building libstd for Miri on macOS)
 - rust-lang#77295 (Update unstable-book: Fix ABNF in inline assembly docs)

Failed merges:

r? `@ghost`
@bors bors merged commit 734c57d into rust-lang:master Sep 28, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 28, 2020
@poliorcetics poliorcetics deleted the ui-to-unit-test-1 branch September 28, 2020 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

Assignees

@matklad matklad

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
6 participants
@poliorcetics @bors @matklad @rust-highfive @jyn514 @rustbot