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

Improve doc cfg(test) and tests directory #38823

Merged
merged 1 commit into from
Feb 3, 2017

Conversation

Freyskeyd
Copy link
Contributor

Hi,

I was facing a problem with my code organisation. I was using a tests directory and i defined some #[cfg(test)] in my src/. But i was not able to use it in my tests folder.

.
├── Cargo.lock
├── Cargo.toml
├── src
│   ├── lib.rs
│   └── test.rs
└── tests
    └── x.rs

src/lib.rs

#[cfg(test)]
pub mod test;

#[test]
fn tesst() {
    assert!(test::t());
}

src/test.rs

pub fn t() -> bool { true }

test/x.rs

extern crate testt;

use testt::test;
#[test]
fn tesst() {
    assert!(test::t());
}

I was unable to compile using cargo test:

error[E0432]: unresolved import `testt::test`
 --> tests/x.rs:3:5
  |
3 | use testt::test;
  |     ^^^^^^^^^^^ no `test` in `testt`

If i remove the tests directory everything works fine. To use an utils module in your tests directory, you need to create a module in the directory (like tests/utils.rs). My tests/x.rs look like this now:

extern crate testt;

mod utils;

#[test]
fn tesst() {
    assert!(utils::t());
}

And my tree:

.
├── Cargo.lock
├── Cargo.toml
├── src
│   └── lib.rs
└── tests
    ├── utils.rs
    └── x.rs

I think that thing must be documented in the book.

Ping:

  • @badboy : Because he's the one who showed me the path
  • @shahn : Because he helped me too to find the solution

Signed-off-by: Freyskeyd [email protected]

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@GuillaumeGomez
Copy link
Member

I don't like very much the way you worded it. :-/ #[cfg(test)] isn't ignored, this is conditional compilation so it cannot be ignored.

@Freyskeyd
Copy link
Contributor Author

@GuillaumeGomez right.

What do you think:

Note, every `#[cfg(test)]` in the `src` directory will not be compiled because...

@badboy
Copy link
Member

badboy commented Jan 4, 2017

I do not like this addition.

The section already mentions:

This is because each test in the tests directory is an entirely separate crate, and so we need to import our library.

Maybe we should just stress a bit more that this means that the main crate is not compiled in test mode?

@Freyskeyd
Copy link
Contributor Author

Something like:

This is because each test in the `tests` directory is an entirely separate crate, and so we need to import our library.
Meaning that every `#[cfg (test)]` will not be compiled in the main crate.

cc @badboy

@GuillaumeGomez
Copy link
Member

Hum, I'm shared on this. :-/

cc @rust-lang/docs

@frewsxcv
Copy link
Member

frewsxcv commented Jan 9, 2017

I'm not a fan of the proposed wording for this, but I can't think of anything better to suggest right now. I'll think about this...

@steveklabnik
Copy link
Member

steveklabnik commented Jan 23, 2017

Here's how I'd word this:

Note, when building integration tests, your library is compiled without cfg(test), since it's an external test, and so is testing your library as consumers of that library would use it.

WDTY @rust-lang/docs ?

@GuillaumeGomez
Copy link
Member

@steveklabnik: I don't like your wording either. Too confusing.

@steveklabnik
Copy link
Member

Could you elaborate on what specifically is confusing?

@GuillaumeGomez
Copy link
Member

Sure!

your library is compiled without cfg(test)

I think a better wording would be:

since the test flag isn't passed to the compiler, all parts in cfg(test) won't be included in the build

@steveklabnik
Copy link
Member

I like that too. @Freyskeyd what do you think?

@Freyskeyd
Copy link
Contributor Author

That's nice, what do you think of:

Note, when a tests folder exists, cargo will not pass the testflag to the compiler, all parts in cfg(test) won't be included in the build.

@frewsxcv
Copy link
Member

Note, when a tests folder exists, cargo will not pass the testflag to the compiler, all parts in cfg(test) won't be included in the build.

@Freyskeyd The underlying issue here is not that a certain directory exists, but that when running integration tests, the library crate being tested will not be compiled with cfg(test), which some users might be confused by.


Regarding @steveklabnik's suggestion:

Note, when building integration tests, your library is compiled without cfg(test)...
I like this part.

I like this part.

...since it's an external test, and so is testing your library as consumers of that library would use it.

I like the direction of this: indicating the library is being built in the same manner as a consumer might build it, but I'm not much a fan of the wording. I've read it through a dozen times and still don't really get it. I might be too sleepy right now though 😴 .

This is clearer to me, albeit maybe a bit wordy:

Note, when building integration tests, your library is compiled without cfg(test). Building your library without the test attribute better emulates how a consumer of your library might build it.

@GuillaumeGomez
Copy link
Member

@frewsxcv: your sentence has the same issue:

your library is compiled without cfg(test)

This is how it works, but it'd be great to include why, like I did when I proposed a modified version of what @steveklabnik suggested.

@Freyskeyd
Copy link
Contributor Author

@GuillaumeGomez what do you think:

Note, when building integration tests, cargo will not pass the test flag to the compiler. It means that all parts in cfg(test) won't be included in the build used in your integration tests.

@GuillaumeGomez
Copy link
Member

@Freyskeyd: Great! 👍

@Freyskeyd Freyskeyd force-pushed the doc-missingInformationCfgTest branch 2 times, most recently from be64366 to eeae310 Compare January 24, 2017 15:23
@@ -499,6 +499,10 @@ be imported in every test with `mod common;`
That's all there is to the `tests` directory. The `tests` module isn't needed
here, since the whole thing is focused on tests.

Note, when building integration tests, cargo will not pass the test flag to the
compiler. It means that all parts in cfg(test) won't be included in the build
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

`cfg(test)`

@@ -499,6 +499,10 @@ be imported in every test with `mod common;`
That's all there is to the `tests` directory. The `tests` module isn't needed
here, since the whole thing is focused on tests.

Note, when building integration tests, cargo will not pass the test flag to the
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

cargo will not pass the `test` attribute

https://doc.rust-lang.org/book/testing.html#the-test-attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frewsxcv you want me to add a link to this page or add single quote around test?

Copy link
Member

Choose a reason for hiding this comment

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

Adding the grave accents and calling it an "attribute" instead of a "flag", since TRPL calls it an "attribute".

Copy link
Member

Choose a reason for hiding this comment

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

The reference also calls them "attributes":

https://doc.rust-lang.org/reference.html#function-only-attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but in conditional-compilation it's a flag:

test - Enabled when compiling the test harness (using the --test flag).

Copy link
Member

Choose a reason for hiding this comment

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

That's referring to --test, which is a command line argument, also called a "flag"

https://en.wikipedia.org/wiki/Command-line_interface#Command-line_option

@Freyskeyd Freyskeyd force-pushed the doc-missingInformationCfgTest branch from eeae310 to b996a7e Compare January 24, 2017 20:04
@Freyskeyd
Copy link
Contributor Author

Freyskeyd commented Jan 30, 2017

@steveklabnik What do you think ? :)

@steveklabnik
Copy link
Member

Looks fine to me, let's :shipit:

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 30, 2017

📌 Commit b996a7e has been approved by steveklabnik

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 2, 2017
…Test, r=steveklabnik

Improve doc cfg(test) and tests directory

Hi,

I was facing a problem with my code organisation. I was using a tests directory and i defined some `#[cfg(test)]` in my `src/`. But i was not able to use it in my `tests` folder.

```bash
.
├── Cargo.lock
├── Cargo.toml
├── src
│   ├── lib.rs
│   └── test.rs
└── tests
    └── x.rs
```
> src/lib.rs
```rust
pub mod test;

fn tesst() {
    assert!(test::t());
}
```
> src/test.rs
```rust
pub fn t() -> bool { true }
```
> test/x.rs
```rust
extern crate testt;

use testt::test;
fn tesst() {
    assert!(test::t());
}
```

I was unable to compile using `cargo test`:
```bash
error[E0432]: unresolved import `testt::test`
 --> tests/x.rs:3:5
  |
3 | use testt::test;
  |     ^^^^^^^^^^^ no `test` in `testt`
```

If i remove the `tests` directory everything works fine. To use an utils module in your `tests` directory, you need to create a module in the directory (like `tests/utils.rs`). My `tests/x.rs` look like this now:

```rust
extern crate testt;

mod utils;

fn tesst() {
    assert!(utils::t());
}
```

And my tree:
```bash
.
├── Cargo.lock
├── Cargo.toml
├── src
│   └── lib.rs
└── tests
    ├── utils.rs
    └── x.rs
```

I think that thing must be documented in the book.

Ping:
- @badboy : Because he's the one who showed me the path
- @shahn : Because he helped me too to find the solution

Signed-off-by: Freyskeyd <[email protected]>
bors added a commit that referenced this pull request Feb 2, 2017
Rollup of 9 pull requests

- Successful merges: #38823, #39196, #39299, #39319, #39373, #39383, #39416, #39420, #39427
- Failed merges:
@bors bors merged commit b996a7e into rust-lang:master Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants