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

cargo test incorrectly warns for dead code #46379

Open
jean553 opened this issue Nov 29, 2017 · 15 comments
Open

cargo test incorrectly warns for dead code #46379

jean553 opened this issue Nov 29, 2017 · 15 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.

Comments

@jean553
Copy link

jean553 commented Nov 29, 2017

Hi!

I got unexpected dead code warnings when executing cargo test with multiple test files. I use a very simple example below that reproduces the issue.

I have two files that contains tests, tests/test_one.rs and tests/test_two.rs. Both contains exactly the same content (except for the unique test function name they contain):

mod routines;

#[test]
fn test_one() {
    routines::my_routine();
}

And another file called tests/routines.rs that simply contains:

pub fn my_routine() {
}

When I execute cargo test, the two tests are executed successfully and there is no raised warning. But if I remove the my_routine() call from one of the two tests, cargo test stills end up successfully but raises a warning on pub fn routine() saying function is never used. However, one test still calls the function, so there is no dead code as stated.

I got the same issue with both rust stable (1.22.1) and rust nightly (1.24.0).

Thanks.

@steveklabnik
Copy link
Member

This is not actually a bug, as far as I know!

And another file called tests/routines.rs that simply contains:

Cargo is also attempting to compile this file as its own test. See the explanation here: https://doc.rust-lang.org/book/second-edition/ch11-03-test-organization.html#submodules-in-integration-tests

I believe if you move this file to tests/routines/mod.rs, this will work without warning.

@jean553
Copy link
Author

jean553 commented Nov 29, 2017

Yes, when replacing tests/routines.rs by tests/routines/mod.rs, Cargo does not attempt to compile routines.rs as a test.

But, the warning is still raised at the first execution of cargo test (and only at the first execution).

Here the details of what I do after changing tests/routines.rs to tests/routines/mod.rs:

  1. execute cargo test multiple times with two calls to my_routine() (one in each test) -> no warning
  2. remove one call to my_routine() from one of the two tests
  3. execute cargo test -> one warning function is never used is raised for my_routine()
  4. execute cargo test again (without code modification) -> no warning anymore

For exactly the same code, cargo test outputs a warning only during the first execution. Furthermore, for each execution, the two tests are run so my_routine() is called. Maybe I just miss something, but this is kind of weird, isn't it ?

@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. labels Jan 23, 2018
@nickolay
Copy link
Contributor

I believe cargo compiles each "integration test" (i.e. file in tests/*.rs) independently. So if the helper in a common module tests/routines/mod.rs is used in tests/test_one.rs, but not in tests/test_two.rs, there will be a warning when compiling test_two.rs.

(cargo test re-builds only what needs to be rebuilt, that's why it only warns on the first execution.)

I've worked around this by using mod routines; pub use routines::*; in tests/*.rs.

@svenstaro
Copy link

So we're running into the same issue and I think the workaround mod routines; pub use routines::*; is not optimal as that does hide truly dead code. I think this is a legitimate cargo bug.

@KSXGitHub
Copy link

This is either a bug or a design flaw.

In TypeScript, an exported item (even if it is never used) is never considered dead code. I don't why it is in Rust.

@nickolay
Copy link
Contributor

@KSXGitHub well, if you export the items from the crate with mod routines; pub use routines::*; it's not considered dead code.

What @svenstaro is asking for would require considering all targets/configurations before determining if an item is dead code instead of letting the rust compiler report them when compiling an individual crate...

@jweinst1
Copy link

jweinst1 commented May 3, 2019

I am noticing this issue,

when will the PR above be released in rust ? ^

@eftychis
Copy link

We are encountering the same issue. I am not sure I am comfortable with no dead code warnings. Perhaps an alternative approach to organizing code people would suggest if there is no re-design any time soon?

0xjac pushed a commit to TrueLevelSA/core-cbc-casper that referenced this issue Oct 13, 2019
0xjac pushed a commit to TrueLevelSA/core-cbc-casper that referenced this issue Oct 24, 2019
@pradovic
Copy link

pradovic commented Nov 1, 2019

If the module is implemented in tests/routines/mod.rs, just using pub mod routines;, in every test file that I wish to use routines module, solved the problem for me. This works as expected, imo, because:

  1. It allows to declare a shared module for different tests
  2. Does not report unused code warning if you don't use all the functions from the shared module, which is good, imo
  3. Does not report unused code warning in the test file that does not use routines at all.

So, it looks to me that exported stuff is also not considered a dead code in rust, but it needs to be declared/imported with pub.
There is a nice example of the similar use-case in the rust book: https://doc.rust-lang.org/1.29.2/book/2018-edition/ch07-02-controlling-visibility-with-pub.html.

@bachrc
Copy link

bachrc commented May 27, 2020

We are still encountering this issue with the last version of Rust (1.45.0 nightly)

brandonkal added a commit to brandonkal/inkjet that referenced this issue Aug 4, 2020
rouge8 added a commit to rouge8/jit.rs that referenced this issue Jun 3, 2021
Previously `cargo test` would wrongly complain about dead code in
`tests/common.rs`. This patch adds a workaround from
rust-lang/rust#46379.
@X01XX
Copy link

X01XX commented Dec 24, 2021

Allowing dead code warnings, and running "cargo test", seems to show truly dead code warnings.

Mathspy added a commit to Mathspy/diary-generator that referenced this issue Jan 15, 2022
Not ideal but this will still catch tests not marked with
using a test utility that we won't be told about it by Ferris, sadly

See: rust-lang/rust#46379
Mathspy added a commit to Mathspy/diary-generator that referenced this issue Jan 15, 2022
* Extract new_page test utility to utils module

* Derive the temp dirs names from test name automatically

* Allow dead_code inside of utils to avoid unactionable warnings

Not ideal but this will still catch tests not marked with
using a test utility that we won't be told about it by Ferris, sadly

See: rust-lang/rust#46379
woodruffw added a commit to woodruffw/kbs2 that referenced this issue Mar 9, 2022
sanket1729 added a commit to sanket1729/rust-miniscript that referenced this issue May 26, 2022
Future commits will remove the hacky integration test setup. There are
still some annoying warnings present for ununsed functions when they are
really used.
rust-lang/rust#46379
sanket1729 added a commit to sanket1729/rust-miniscript that referenced this issue May 26, 2022
Future commits will remove the hacky integration test setup. There are
still some annoying warnings present for ununsed functions when they are
really used.
rust-lang/rust#46379

Disambiguiate rand feature with dev dependancy

https://users.rust-lang.org/t/features-and-dependencies-cannot-have-the-same-name/47746
woodruffw added a commit to woodruffw/kbs2 that referenced this issue May 27, 2022
* Initial integration tests

* hackety hack

* Cleanup, fix dead code warnings

See rust-lang/rust#46379.

* tests: more integration tests
sanket1729 added a commit to rust-bitcoin/rust-miniscript that referenced this issue Jun 2, 2022
139324a Remove integration test code (sanket1729)
90b5f10 Remove warnings (sanket1729)
967b95e Add support for running integration tests via cargo test (sanket1729)

Pull request description:

  There are still some annoying warnings present for unused functions when they are used.
  rust-lang/rust#46379

  Later commits will remove the hacky integration test setup.

  Running `cargo test` now should also run integration tests

  Our testing infrastructure now takes a long time to build because we have more than 100 dependencies (transitive). But all of these are dev dependencies, so our library isn't getting bloated.

  Fixes #361

ACKs for top commit:
  apoelstra:
    ACK 139324a

Tree-SHA512: debf68fd0ac98cffa9c8c89964d6d1c45ee542fe17eb2fdab6295357efc06eb43a0412e6ed5d0fd7264a987989984eebb0c5b3bbf7169ecb780d31dce887cb0b
joemphilips pushed a commit to joemphilips/rust-miniscript that referenced this issue Dec 9, 2022
Future commits will remove the hacky integration test setup. There are
still some annoying warnings present for ununsed functions when they are
really used.
rust-lang/rust#46379

Disambiguiate rand feature with dev dependancy

https://users.rust-lang.org/t/features-and-dependencies-cannot-have-the-same-name/47746
@xixixao
Copy link

xixixao commented Jan 7, 2023

This is still a problem, with any test not using all helpers cargo reports the helpers as dead code.

jamesbornholt added a commit to awslabs/mountpoint-s3 that referenced this issue Jan 16, 2023
In #33 we saw weirdness adding code to `tests/common/mod.rs` creating
dead code warnings. This is because of Cargo's slightly funky rules for
compiling integration test modules
(rust-lang/rust#46379). We can fix it by
claiming the module is public.
lukenels pushed a commit to awslabs/mountpoint-s3 that referenced this issue Jan 16, 2023
In #33 we saw weirdness adding code to `tests/common/mod.rs` creating
dead code warnings. This is because of Cargo's slightly funky rules for
compiling integration test modules
(rust-lang/rust#46379). We can fix it by
claiming the module is public.
nitnelave added a commit to lldap/lldap that referenced this issue May 2, 2023
We're running afoul of rust-lang/rust#46379,
where each test is compiled independently, so any test that doesn't use
every helper method triggers a dead code warning.
nitnelave added a commit to lldap/lldap that referenced this issue May 2, 2023
We're running afoul of rust-lang/rust#46379,
where each test is compiled independently, so any test that doesn't use
every helper method triggers a dead code warning.
viiri pushed a commit to viiri/exepack that referenced this issue May 14, 2023
Print "ok" and exit cleanly, rather than run off the end of a bunch of
nops or nil bytes.

DOSBox still cannot run some text executables for other reasons, such as
(I suspect) large stack pointers or e_minalloc values.

I had to do `pub` on the imports of the common module in tests, to avoid
dead_code lints.
rust-lang/rust#46379
CaviarChen added a commit to MemoLanes/MemoLanes that referenced this issue Dec 17, 2023
Avoid the dead code false alarm + avoid it show up in tests.
The solution is not super ideal but apparently there isn't anything
better: rust-lang/rust#46379
@ik1ne
Copy link

ik1ne commented Jan 1, 2024

The rust book's 11.3. Test Organization encourages to create tests/common/mod.rs for test setup codes.

However, this results to false positive in some cases such as common::common_func1() is only used by tests/test1.rs and common::common_func2() is only used by tests/test2.rs.

@CobaltCause
Copy link

A workaround for this issue is to use the tests/it/main.rs pattern as described by https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html (although I usually spell it tests/integrations/main.rs in my own projects).

@ClementTsang
Copy link
Contributor

ClementTsang commented Jan 15, 2024

+1 to following the tests/it/main.rs pattern in https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html as a workaround for this issue.

sciprosk added a commit to sciprosk/bernstein that referenced this issue Jun 2, 2024
The issue with dead code compiler warnings in tests is desribed
here: rust-lang/rust#46379.
Bastian pushed a commit to Bastian/bstats-data-processor that referenced this issue Jul 25, 2024
Bastian pushed a commit to Bastian/bstats-data-processor that referenced this issue Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests