Make all tests functions and structs crate-private #2735
Conversation
|
I found a lot of methods and variables that are not being used, should I remove them all? cc: @kinnison @rbtcollins |
0xPoe
force-pushed
the
rustin-patch-cleanup
branch
from
cc1a437 to
3192949
Compare
|
/hold Waiting for anyhow wroks laned. |
0xPoe
force-pushed
the
rustin-patch-cleanup
branch
from
3192949 to
9cb5b52
Compare
|
/unhold I think it's ready for review. |
|
I'm not entirely sure if this is valuable here. The way our tests are structured makes it quite hard to know for sure about which bits are used in which test files. Also I'm going to defer to @rbtcollins for this particular PR. |
|
Because our test is a standalone crate, it warns on cargo check, but not on cargo clippy. I don't have too good a solution for it. |
|
I wonder if, instead, what we need is to restructure our tests/ dir into tests/dist.rs and tests/cli.rs with clitools/ and clitests/ as subdirs - i.e. build the dist tests (which import the rustup module) and the cli tests as two integration binaries rather than the nearly 10 we have now. Then the clitools stuff becomes a module used by only one binary (the cli tests) and as such controlling unused functions etc. becomes much more plausible. If we did that though, we'd need to be sure that we'd bottomed the potentially dodgy parallelism in some of the tests. |
| /// Where we put the rustup / rustc / cargo bins | ||
| pub exedir: PathBuf, | ||
| pub(crate) exedir: PathBuf, |
There was a problem hiding this comment.
we shouldn't do this - just the pub(crate) on the struct, per our discussions and that bug.
There was a problem hiding this comment.
No, if we remove the modifier, it will be private. The bug fixed is the use of pub in pub(crate).
There was a problem hiding this comment.
Could this be another bug?
There was a problem hiding this comment.
I'm not suggesting private, I'm suggesting simple pub
There was a problem hiding this comment.
Oh, you mean in this case we don't need to change it to pub(crate) but keep it pub?
|
Exactly
…On Mon, 31 May 2021, 13:54 二手掉包工程师, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/mock/clitools.rs
<#2735 (comment)>:
> /// Where we put the rustup / rustc / cargo bins
- pub exedir: PathBuf,
+ pub(crate) exedir: PathBuf,
Oh, you mean in this case we don't need to change it to pub(crate) but
keep it pub?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2735 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZ7XUFY5TDLDFHYLCEXX3TQN2I3ANCNFSM43SFUNGQ>
.
|
|
Close it for now, as it causes cargo check to misrepresent a lot of unused warnings. It can be very disruptive until we can sort out and re-architect the tests. |
part of #2730