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

Semantic errors ignored in tests with cargo check --tests and cargo build --tests #4003

Closed
tbourvon opened this issue May 7, 2017 · 5 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-check

Comments

@tbourvon
Copy link

tbourvon commented May 7, 2017

Cargo version: cargo 0.19.0-nightly (fa7584c14 2017-04-26)

cargo check --tests and cargo build --tests seem to ignore semantic errors in tests.

I don't know if that's intentional, but it's frustrating to be unable to cargo check code in my tests properly.

Examples:

Common Cargo.toml

[package]
name = "check_test"
version = "0.1.0"
authors = ["me"]

[dependencies]

  • src/lib.rs example 1
#[cfg(test)]
mod tests {
    #[test]
    fn it_works() {
        asset_eq!(1, 1); // intentional typo (semantic error)
    }
}

What happens: running cargo check --lib --tests completes with no errors (code 0)

What should happen: running cargo check --lib --tests should indicate:

cannot find macro `asset_eq!` in this scope
  help: did you mean `assert_eq!`?

  • src/lib.rs example 2
fn not_a_test() {
    asset_eq!(1, 1); // intentional typo (semantic error)
}

#[cfg(test)]
mod tests {
    #[test]
    fn it_works() {
        
    }
}

What happens AND should happen: running cargo check --lib --tests indicates:

cannot find macro `asset_eq!` in this scope
  help: did you mean `assert_eq!`?

  • src/lib.rs example 3
#[cfg(test)]
mod tests {
    #[test]
    fn it_works() {
        i'm syntaxically incorrect code
    }
}

What happens AND should happen: running cargo check --lib --tests (actually cargo check gives the same output, so I think that syntax is checked regardless of cargo filters) indicates:

message: 'expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `'m`
  label: expected one of 8 possible tokens here'
@alexcrichton
Copy link
Member

Thanks for the report! This is actually expected, albeit somewhat odd, behavior. What's happening here is that we're actually never compiling the library in test mode! Specifically when you pass --tests that actually only includes integration tests right now (e.g. tests/*.rs) not the library tests as well. Additionally the --lib flag means "the compiled library", not "the tested library".

I think there's two possible fixes to this issue:

  • Like cargo rustc, we could add a --profile flag to check. This means that to check the library you'd execute cargo check --profile test --lib
  • We could also just add the library's tests to the --tests flag.

I'd personally be in favor of the latter, it seems to be the most reasonable! This is actually I think a relatively easy issue to tackle, would you be interested in sending a PR? I could help guide you through where the code is located and how to write a test!

@tbourvon
Copy link
Author

tbourvon commented May 9, 2017

@alexcrichton Thank you for your answer.

Actually, I would be thrilled to work on this issue, and I agree with you on the second option. Is there a way I can contact you so that we can talk about that?

@alexcrichton
Copy link
Member

Oh awesome! We can either coordinate here or I'm also on IRC in #cargo on irc.mozilla.org as acrichto, either way is fine by me!

@carols10cents
Copy link
Member

Related PR: #4039

@carols10cents carols10cents added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-check labels Oct 2, 2017
ehuss added a commit to ehuss/cargo that referenced this issue Oct 15, 2017
- Add `--profile test` flag to `cargo check` that will enable checking of unit tests.
- `--tests` will implicitly enable checking of unit tests within the lib.
- Don't implicitly compile binaries when using just `--test` filters.
- Fix erroneously linking tests when run with `--test`.

Fixes rust-lang#3431, rust-lang#4003, rust-lang#4059, rust-lang#4330.
bors added a commit that referenced this issue Oct 30, 2017
Add unit test checking to `cargo check`

This is an extension of PR #4039, fixing #3431, #4003, #4059, #4330.  The fixes for #4059 can potentially be separated into a separate PR, though there may be some overlap.

The general gist of the changes:

- Add `--profile test` flag to `cargo check` that will enable checking of unit tests.
- `--tests` will implicitly enable checking of unit tests within the lib (checks the same targets as `cargo test`).  This affects the `check`, `test`, and `build` commands.
- `--benches` behaves similarly by using the same targets as `cargo bench`.
- Fix erroneously linking tests when run with `--test`.

There is one thing I did not do because I wanted more feedback on what others think the expected behavior should be.  What should the behavior of `--all-targets` be?  This patch does not (yet) make any changes to its behavior.  My initial thinking is that it should *add* a target of `--lib --bins --profile test`, but that essentially means the lib and bin targets will be checked twice (and thus any errors/warnings outside of `#[cfg(test)]` will appear twice, which may be confusing, and generally take twice as long to run).  I can add that, but I think it would require adding a new `All` variant to `CompileFilter` so that the code in `generate_targets` can detect this scenario.  I wanted feedback before making a more extensive change like that.  The downside of not adding it is that `--all-targets` will ignore unit tests (if you don't specify `--profile test`).

Summary of the profiles used with this patch:

Command                         | Lib               | Bin foo     | Test t1 | Example e1 | Bench b1 |
-------                         | ---               | -------     | ------- | ---------- | -------- |
`check`                         | check             | check       | -       | -          | -        |
`check --profile test`          | check_test†       | check_test† | -       | -          | -        |
`check --lib`                   | check             | -           | -       | -          | -        |
`check --lib --profile test`    | check_test†       | -           | -       | -          | -        |
`check --bin foo`               | check             | check       | -       | -          | -        |
`check -–bin foo –profile test` | check_test†       | check_test† | -       | -          | -        |
`check --bins`                  | check             | check       | -       | -          | -        |
`check --test t1`               | check             | check       | check_test   | -          | -        |
`check --tests`                 | check, check_test†  | check, check_test† | check_test | check†, check_test†  | -    |
`check --all-targets`           | check, check_test†  | check, check_test†  | check_test   | check, check_test† | check_test    |

† = different behavior from today
@ehuss
Copy link
Contributor

ehuss commented Oct 30, 2017

This is fixed by #4592. --tests now does all tests, including unit tests. Also, cargo check now has a --profile flag that can be used to target unit tests individually, such as cargo check --lib --profile test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-check
Projects
None yet
Development

No branches or pull requests

4 participants