-
Notifications
You must be signed in to change notification settings - Fork 233
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
Testing 1/3: Add a bunch of unit tests #135
Conversation
caa37f1
to
de81fda
Compare
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.
Just a couple FIXME
comments. Otherwise this looks awesome.
@@ -24,6 +24,7 @@ pub struct NodeDistro { | |||
|
|||
/// Check if the cached file is valid. It may have been corrupted or interrupted in the middle of | |||
/// downloading. | |||
// FIXME(#134) - verify checksum |
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 citing an issue! Let's use consistent formatting and write ISSUE(#134)
instead of FIXME
.
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.
done
src/command/shim.rs
Outdated
@@ -108,6 +108,7 @@ Options: | |||
} | |||
} | |||
|
|||
// FIXME: all the logic here should be moved to notion-core |
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.
One last FIXME
left :)
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.
done
@@ -166,3 +181,173 @@ where | |||
Ok(bin_map) | |||
} | |||
} | |||
|
|||
#[cfg(test)] | |||
pub mod tests { |
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.
I don't have a strong opinion either way, but you could consider splitting these out into a separate file. You'd create a manifest/
subdirectory, move manifest.rs
into manifest/mod.rs
, split out this module into manifest/test.rs
, and then change this code to
#[cfg(test)]
pub mod tests;
But I don't know if that's better or worse, just a thought.
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.
Good idea. And since you already refactored manifest.rs to manifest/mod.rs, this is an easy change
de81fda
to
bc113ac
Compare
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 long last—take that, 32-bit msvc. 🕺
As the first part of improving the testing for Notion, I have added a bunch of unit tests.
Next up is creating an acceptance testing harness to tackle #7.