rust: replace static fixture with test helpers#4433
Merged
Conversation
Summary: In #4385, @nfelt suggested using test helpers to easily populate a commit rather than using a single fixture across all test cases. This patch implements such test helpers and uses them in the server tests. The net diff to the test code (not the helpers) is −32 lines. Test Plan: Existing tests pass. The new test helpers mostly don’t have explicit tests, but that’s okay since they’re exercised by the main test code. wchargin-branch: rust-test-data-builders wchargin-source: 31164b4d2b4a1fb9bbabc584249e1d3715c17a3e
wchargin-branch: rust-test-data-builders wchargin-source: 1120046a43711e742fd50ff81266a04e45359136
wchargin-branch: rust-test-data-builders wchargin-source: 1d53aecf45310e9c02513469074c7ee4b6675c74
wchargin
added a commit
that referenced
this pull request
Dec 8, 2020
Summary: Some time between 13:41 and 17:20 Pacific time today, Travis stopped even pretending to run our builds. Note: #4433 reports a Travis status, whereas #4434 has no such item, not even “pending”. And our Travis dashboard shows no builds more recent than #4433, either. Relevant Hacker News: <https://news.ycombinator.com/item?id=25338983> Thankfully, we finally landed #2953 on Friday, so we are no longer dependent on Travis. This patch deletes the Travis configs and excises all remaining mentions. Test Plan: Running `git grep -i travis` now fails. wchargin-branch: ci-rm-travis wchargin-source: c61b1e12df8b53e9231bc36f2c99a7bd140843b4
wchargin
added a commit
that referenced
this pull request
Dec 8, 2020
Summary: Some time between 13:41 and 17:20 Pacific time today, Travis stopped even pretending to run our builds. Note: #4433 reports a Travis status, whereas #4434 has no such item, not even “pending”. And our Travis dashboard shows no builds more recent than #4433, either. Relevant Hacker News: <https://news.ycombinator.com/item?id=25338983> Thankfully, we finally landed #2953 on Friday, so we are no longer dependent on Travis. This patch deletes the Travis configs and excises all remaining mentions. Test Plan: Running `git grep -i travis` now fails. wchargin-branch: ci-rm-travis
nfelt
approved these changes
Dec 8, 2020
tensorboard/data/server/commit.rs
Outdated
| fn default() -> Self { | ||
| ScalarTimeSeriesBuilder { | ||
| step_start: Step(0), | ||
| wall_time_start: WallTime::new(1235.0).unwrap(), |
Contributor
There was a problem hiding this comment.
Optional, but left to my own devices I would probably either omit some of these default values (aka require them to be set on the builder) or at least try to have the defaults be sort of the "minimally arbitrary" values (e.g. start wall time at 0.0 incrementing by 1.0, make len 1, make eval just be 0 always, etc.).
Otherwise we still have test_read_scalars() making assertions like assert_eq!(xent_data.wall_time, vec![1235.0, 1236.0, 1237.0]); that can't be understood to be correct without consulting this default implementation. By contrast, test_list_scalars() is explicit at least about the wall_time_start and len values and thus its assertions are self-contained.
Contributor
Author
There was a problem hiding this comment.
Thanks. You have inspired me to be less lazy. Done.
Contributor
There was a problem hiding this comment.
<meme>what is my purpose?</meme>
wchargin-branch: rust-test-data-builders wchargin-source: beb88cf56ad51ae6ec2413bb31f741dcf364573e
wchargin-branch: rust-test-data-builders wchargin-source: beb88cf56ad51ae6ec2413bb31f741dcf364573e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
In #4385, @nfelt suggested using test helpers to easily populate a
commit rather than using a single fixture across all test cases. This
patch implements such test helpers and uses them in the server tests.
The net diff to the test code (not the helpers) is −32 lines.
Test Plan:
Existing tests pass. The new test helpers mostly don’t have explicit
tests, but that’s okay since they’re exercised by the main test code.
wchargin-branch: rust-test-data-builders