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

Update testing.md to reflect changes to cargo new #37368

Merged
merged 6 commits into from
Nov 12, 2016

Conversation

trotter
Copy link
Contributor

@trotter trotter commented Oct 23, 2016

cargo new now creates a src/lib.rs with a tests module by default. I've updated the earlier examples in this doc to reflect this. However, I don't know how we want to approach the "introduction" to idiomatic testing that follows in "the tests module" section. I think it should be broken apart, with the module concept being introduced early on, and the super concept being addressed when we hit the add_two example. I'd like to get agreement on that being the right approach before I do it though.

I also removed the #fn main() {} hidden at the beginning of each example, as these cause Rust Playground to not treat the file as a set of tests that it can run. Removing it should cause Rust Playground to display a "Test >" button in the top left when a user runs the code, which will allow them to see the test runner output.

@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.

@steveklabnik
Copy link
Member

I recently confronted this for the new book's testing chapter: rust-lang/book#288

maybe a similar approach would be good? I think I did what you did, but it's late and I'm tired; will re-read this PR tomorrow.

I also removed the #fn main() {} hidden at the beginning of each example, as these cause Rust Playground to not treat the file as a set of tests that it can run. Removing it should cause Rust Playground to display a "Test >" button in the top left when a user runs the code, which will allow them to see the test runner output.

👍 IIRC this was due to a limitation of long-ago Rust.

@ollie27
Copy link
Member

ollie27 commented Oct 24, 2016

The # fn main() {} is to stop rustdoc from surrounding all of the code in a fn main block. See #31249. Removing it will generate links like this which still doesn't work. One (still hacky) solution would be to change it to somthing like # // fn main which will generate links like this instead.

@steveklabnik
Copy link
Member

Ah, interesting.

@rust-lang/docs what do you think we should do here?

@trotter
Copy link
Contributor Author

trotter commented Oct 24, 2016

Ahh cool. I didn't realize there was a new book underway. I'm happy to take a look at that and use it as an example for making the existing docs reflect the current state of cargo new.

Once we have clarity on what to do around # fn main() {}, I'm happy to go implement 😄

@bors
Copy link
Contributor

bors commented Nov 2, 2016

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

mod tests {
#[test]
fn it_works() {
}
Copy link
Member

Choose a reason for hiding this comment

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

bad indent

@GuillaumeGomez
Copy link
Member

@steveklabnik: I like these changes so 👍 for me.

@steveklabnik
Copy link
Member

@trotter I would like to merge this, can you rebase and fix that spacing issue @GuillaumeGomez pointed out?

`cargo new` now creates a `src/lib.rs` with a `tests` module by default. I've updated the earlier examples in this doc to reflect this. However, I don't know how we want to approach the "introduction" to idiomatic testing that follows in "the tests module" section. I _think_ it should be broken apart, with the module concept being introduced early on, and the `super` concept being addressed when we hit the `add_two` example. I'd like to get agreement on that being the right approach before I do it though.

I _also_ removed the `#fn main() {}` hidden at the beginning of each example, as these cause Rust Playground to not treat the file as a set of tests that it can run. Removing it _should_ cause Rust Playground to display a "Test >" button in the top left when a user runs the code, which will allow them to see the test runner output.
@trotter
Copy link
Contributor Author

trotter commented Nov 8, 2016

@steveklabnik Updated, but I want ensure that I didn't muck things up with the removal of # fn main() {} before you merge.

@@ -78,10 +79,12 @@ So why does our do-nothing test pass? Any test which doesn't `panic!` passes,
and any test that does `panic!` fails. Let's make our test fail:

```rust
# fn main() {}
Copy link
Member

Choose a reason for hiding this comment

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

so, testing this, i still get a link like

https://play.rust-lang.org/?code=fn%20main()%20%7B%7D%0A%23%5Btest%5D%0Afn%20it_works()%20%7B%0A%7D%0A

which has the main, not wrapping the example.

@steveklabnik
Copy link
Member

@ollie27 i'm not seeing the behavior you're talking about, am i missing something?

@ollie27
Copy link
Member

ollie27 commented Nov 8, 2016

# fn main() {}
#[test]
fn it_works() {}

produces: https://play.rust-lang.org/?code=fn%20main()%20%7B%7D%0A%23%5Btest%5D%0Afn%20it_works()%20%7B%7D%0A

#[test]
fn it_works() {}

produces: https://play.rust-lang.org/?code=fn%20main()%20%7B%0A%23%5Btest%5D%0Afn%20it_works()%20%7B%7D%0A%7D

# // please don't add fn main
#[test]
fn it_works() {}

produces: https://play.rust-lang.org/?code=%2F%2F%20please%20don%27t%20add%20fn%20main%0A%23%5Btest%5D%0Afn%20it_works()%20%7B%7D%0A

Only the third one shows the "Test ▶" button in the playground. Just removing # fn main() {} causes the whole thing to be surrounded in a fn main block like the second example.

@steveklabnik
Copy link
Member

Oh, the isuse is the test button not being valid.

@trotter can you take away the # main() stuff?

@trotter
Copy link
Contributor Author

trotter commented Nov 9, 2016

@steveklabnik Yep! That's what I'm planning on getting fixed before saying we're done 😄

Without these changes, play.rust-lang.org (as of today) would wrap
our examples in `fn main() {}`. This prevents the user from being able
to easily run the tests.
The narrative flows better if we follow what @steveklabnik is doing in
rust-lang/book#288. Therefore, I completely copied it.
I had used `/tmp/adder` for my previous commits. Flipped over to
`/home/you/projects/adder` for consistency with other parts of testing.md
@trotter
Copy link
Contributor Author

trotter commented Nov 10, 2016

@steveklabnik This should be good to merge now. I've fixed the examples such that play.rust-lang.org will correctly run the code as tests. I also removed my earlier additions of mod tests in favor of the approach you took in rust-lang/book#288. Let me know if anything else needs changing.

@GuillaumeGomez
Copy link
Member

Well, still remain a good big squash. :p

@steveklabnik
Copy link
Member

Whoah, it just looks for fn main() and not something syntactically valid? interesting...

@trotter
Copy link
Contributor Author

trotter commented Nov 10, 2016

Fun. I had been running make docs, but apparently missed these tests that are now failing. I'll investigate tonight most likely.

@trotter
Copy link
Contributor Author

trotter commented Nov 10, 2016

I went ahead and took a quick look. It appears that the doc tests are attempting to run the code and are failing because it has no main function. I see two quick ways to making it happy:

  1. remove the comments from # // fn main and turn it into # fn main() {}. I don't like this though, as it results in not having the 'test' button in play.rust-lang.org.
  2. add ,ignore to the markdown language indicator, which will cause the test runner to ignore these ones. This seems good, as it results in things displaying happily to person reading the documentation and the tests pass. It also seems bad though, in that this code isn't getting exercised by the test runner. This means that any typos that show up later will only be found by sad users telling us that the code didn't run when they ran it on play.rust-lang.org.

My preference is to go with number 2 for now, but I'd also love to know if anyone has thoughts on how to get make check-docs to run these code examples without having an fn main() {} around the whole thing.

@alexcrichton alexcrichton added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Nov 10, 2016
@steveklabnik
Copy link
Member

steveklabnik commented Nov 10, 2016

I am also leaning towards ignore. @rust-lang/docs ?

While the commit message on this one sounds terrible, it's really not so
bad. The issue is that our test runner _expects_ a `fn main() {}` in
code blocks that it'll test, but this code really shouldn't have them.
If it did, then clicking the "play" link in the docs would result in
play.rust-lang.org not treating this code as a test example to be run.
@trotter
Copy link
Contributor Author

trotter commented Nov 11, 2016

@steveklabnik OK, I pushed up the changes with ignore. Assuming @rust-lang/docs is cool with it, this should be good to go.

@GuillaumeGomez
Copy link
Member

Considering the current situation, I think it's ok. Thanks for all your work here @trotter!

@steveklabnik
Copy link
Member

@bors: r+ rollup

Yup, let's merge. If anyone else has an issue, please let me know 😄

@bors
Copy link
Contributor

bors commented Nov 11, 2016

📌 Commit 2a832a0 has been approved by steveklabnik

eddyb added a commit to eddyb/rust that referenced this pull request Nov 11, 2016
Update testing.md to reflect changes to cargo new

`cargo new` now creates a `src/lib.rs` with a `tests` module by default. I've updated the earlier examples in this doc to reflect this. However, I don't know how we want to approach the "introduction" to idiomatic testing that follows in "the tests module" section. I _think_ it should be broken apart, with the module concept being introduced early on, and the `super` concept being addressed when we hit the `add_two` example. I'd like to get agreement on that being the right approach before I do it though.

I _also_ removed the `#fn main() {}` hidden at the beginning of each example, as these cause Rust Playground to not treat the file as a set of tests that it can run. Removing it _should_ cause Rust Playground to display a "Test >" button in the top left when a user runs the code, which will allow them to see the test runner output.
eddyb added a commit to eddyb/rust that referenced this pull request Nov 12, 2016
Update testing.md to reflect changes to cargo new

`cargo new` now creates a `src/lib.rs` with a `tests` module by default. I've updated the earlier examples in this doc to reflect this. However, I don't know how we want to approach the "introduction" to idiomatic testing that follows in "the tests module" section. I _think_ it should be broken apart, with the module concept being introduced early on, and the `super` concept being addressed when we hit the `add_two` example. I'd like to get agreement on that being the right approach before I do it though.

I _also_ removed the `#fn main() {}` hidden at the beginning of each example, as these cause Rust Playground to not treat the file as a set of tests that it can run. Removing it _should_ cause Rust Playground to display a "Test >" button in the top left when a user runs the code, which will allow them to see the test runner output.
bors added a commit that referenced this pull request Nov 12, 2016
@bors bors merged commit 2a832a0 into rust-lang:master Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants