----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58706/#review172988 -----------------------------------------------------------
Ship it! 3rdparty/stout/tests/version_tests.cpp Lines 58-63 (original), 70-82 (patched) <https://reviews.apache.org/r/58706/#comment246047> We could simplify this a bit: ``` Try<Version> actual = Version::parse(input); ASSERT_SOME_EQ(std::get<0>(expected), actual) << "Incorrect parse of input '" << input << "'"; EXPECT_EQ(std::get<1>(expected), stringify(actual.get())) << "Incorrect parse of input '" << input << "'"; ``` 3rdparty/stout/tests/version_tests.cpp Lines 86 (patched) <https://reviews.apache.org/r/58706/#comment246036> This reads to me that malformed input strings can be parsed, maybe re-phrase? Also would be nice to use consistent terminology ("malformed" vs "invalid") here 3rdparty/stout/tests/version_tests.cpp Line 64 (original), 86-106 (patched) <https://reviews.apache.org/r/58706/#comment246039> Naming wise, how about: ``` const vector<string> inputs = {...}; foreach (const string& input, inputs) { Try<Version> parse = Version::parse(input); // Or: Try<Version> version = Version::parse(input); ... } ``` Actual seems a bit out of place in this one given there's no "expected" to go along with it. - Benjamin Mahler On April 25, 2017, 8:03 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58706/ > ----------------------------------------------------------- > > (Updated April 25, 2017, 8:03 p.m.) > > > Review request for mesos, Benjamin Mahler and Kapil Arya. > > > Bugs: MESOS-1987 > https://issues.apache.org/jira/browse/MESOS-1987 > > > Repository: mesos > > > Description > ------- > > Switch to table-based test cases, validate that version parsing produces > the expected results more precisely. > > > Diffs > ----- > > 3rdparty/stout/tests/version_tests.cpp > 724ed2292fdd3c5f4c98facf82260078b66a0e97 > > > Diff: https://reviews.apache.org/r/58706/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
