Skip to content

Test and fix to vec-final #86

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

Merged
merged 4 commits into from
Aug 22, 2018
Merged

Conversation

izderadicka
Copy link
Contributor

Fixed Iterator implementation for Drain
Added some tests to code

@RalfJung - as discussed in #71 I've created separate PR for these two changes
@steveklabnik - in #71 you noted that test should run during mdbook test, but it does not seem to be so (if I modify a test to fail nothing happens) - is there anything else needed for tests to run?

Fixed Iterator implementation for Drain
Added some tests to code
@izderadicka
Copy link
Contributor Author

@steveklabnik - I think I get it with tests - mdbook is running doctests only - so I modify code bit to run tests as doctests - also tests are now hidden, as main function is hidden too, so they do not appear in book - to make final example clearer.

@RalfJung
Copy link
Member

@izderadicka so you verified that the tests are now run by mdbook test?

src/vec-final.md Outdated
# for i in v.iter_mut() {
# *i += 1;
# }
# v.insert(0, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe insert a different value here so we can distinguish them, making sure the right one gets indeed popped?

@izderadicka
Copy link
Contributor Author

@RalfJung - Yes test are running now - as doctest is running main and main is running our tests. Also tested that if assertion fails then mdbook test fails as the test panics.

@RalfJung
Copy link
Member

LGTM except for changing v.insert(0, 1); to v.insert(0, 2); (or any value other than 1 and 10).

@izderadicka
Copy link
Contributor Author

@RalfJung - changed test as required

@Gankra Gankra merged commit ae42ad7 into rust-lang:master Aug 22, 2018
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.

3 participants