-
Notifications
You must be signed in to change notification settings - Fork 140
Conversation
What prevented you to use @error256, @darikg, other Rust users I don't know Rust and this was the only way I could think of while maintaining backward compatibility with existing contents. Let me know what you think. |
@kazk Yeah I think it was just back then I overlooked the |
fixture: ` | ||
extern crate rand; | ||
use self::rand::Rng; | ||
use self::rand::distributions::{IndependentSample, Range}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance, it seems unfortunately that we must use this. Would be good to find a way without it before it goes in since once its in will be kinda locked I guess? Unfortunately I can't suggest an alternative just at a glance as I'm not proficient at rust but am happy to tinker a little bit later (Melbourne time 😉) to see if there is any help I can provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing.
Yeah, that's bothering me too.
I also tried having opts.setup
like
#[cfg(test)] extern crate rand;
But this obviously breaks when opts.solution
has extern crate rand;
.
I used Test Organization chapter from "The Rust Programming Language" as a guide.
Currently we can only have one version of "rust-runner" image so we need to be careful not to break existing contents.
am happy to tinker a little bit later (Melbourne time 😉) to see if there is any help I can provide.
Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually went back over it and think it looks really good. Unfortunate that cargo was not used originally. Would have been nice to expect that the functions are going to be defined pub
then could have split it which would have been the best case.
I can see why this is as above, it seems likely people will write the tests though assuming that they won't need the self
on there. At least its probably not that likely for solutions to contain the rand crate generally. I suppose it will impact all use
statements though but it seems like that was already the case?
Features - Use Cargo - Install `rand` package - Improve output - Align with other languages - Include stdout of failed tests - Proper termination Other Changes - Include examples to tests - Restructure tests by context Structure Needs some hack in order to maintain backward compatibility and avoid errors with duplicate import. . |- Cargo.toml `- src |- lib.rs (opt.setup + opts.solution + module declaration) `- tests.rs (opts.fixture) - Placing `opts.fixture` in submodule `tests.rs` should avoid issues with duplicate imports - Maintains backward compatibility by prepending `use super::*;` to `opts.fixture` if missing Another idea was . |- Cargo.toml `- src |- lib.rs (module declarations) |- solution.rs (opts.setup + opts.solution) `- tests.rs (opts.fixture) - Maintaining backward compatibility is hard - Requires solutions to be declared public - Requires tests to import solution `use solution::*;` Using `rand` in tests extern crate rand use self::rand::Rng; Needs `self::` because `extern crate` loads to current name-space and tests are in `tests` module.
This should allow writing random tests using
rand
(#240).I had to come up with a backward compatible way, and I'd like feedbacks from Rust users.
Features
rand
packageOther Changes
Structure
Needs some hack in order to maintain backward compatibility and avoid errors with duplicate import.
opts.fixture
in submoduletests.rs
should avoid issues with duplicate importsuse super::*;
toopts.fixture
if missingAnother idea was
Decided against this because maintaining backward compatibility is hard.
use solution::*;
Using
rand
in testsNeeds
self::
becauseextern crate
loads to current name-space and tests are intests
module.Resolves #240
Closes #462