[list] sort list reply alphabetically (Fixes #836) #994

Saviq · 2019-08-26T13:31:12Z

As part of that, refactor the formatter test suite into a parametrized one.

Take that, C++ tests!

ricab

Nice. Just a suggestion on the tests.

ricab · 2019-08-26T16:48:18Z

tests/test_output_formatter.cpp

+                                 "trusty-190611-1542      Running           --               Ubuntu N/A\n";
+
+    mp::TableFormatter table_formatter;
+    auto output = table_formatter.format(list_reply);


To test treatment of petenv, you could do something like the following (did not actually try it):

EXPECT_CALL(mpt::MockSettings::mock_instance(), get(Eq(mp::petenv_key))).WillOnce(Return("trusty-190611-1539"));

And get that entry first in the expected output trusty-190611-1539.

I tend to keep unit tests single-purpose, this one tests that (non-petenv) instances are sorted alphabetically. I'd like to keep it that way.

If theory we could extend the …_petenv_first tests a bit, but then again those test exactly what they should :)

Maybe we should just add a third test, that tests both sort orders together?

Unit tests are indeed supposed to test "units" of functionality, but where you decide the unit is can often subjective. IMO it would be perfectly appropriate to consider the entire sorting function as the unit here.

The lexicographical ordering is implemented elsewhere, so this really only tests whether it is being called properly on our side. If you then test the petenv comes first without lexicographical ordering of the remaining entries, nothing guarantees that they work together: you could write code that passes both tests without doing both things simultaneously ("if petenv in, put it first, else sort"). IOW the conjunction of two bits of functionality is itself functionality that requires testing.

In practice some parsimony is needed when deciding unit boundaries, lest we end up testing single instructions 😉

I'll just add my $0.02 if anyone cares 😉

I've often struggled with what constitutes a "unit". Is is just a single function? Is it whatever is needed to get an end result, which could be made up of multiple functions? Over time, I've tended to go with the latter.

As such, I agree with @ricab here in that the end result is a list with the petenv always at the top regardless of name and then followed by instance names alphabetically sorted, if any. I really believe there needs be (a) test(s) to ensure the whole result is what we expect. We should have tests that exercise scenarios such as only the petenv exists or the petenv doesn't exist at all and only have other instances.

Sure, which is why I suggested adding another test covering the conjuction of the two. And will do that.

I still find it important to know whether it's the alphabetical sorting, or the petenv precedence, or the conjunction of them failed, so all three tests make sense, IMO.

OK, I am fine with separate testing for the conjunction.

You may want to consider parameterized tests to reduce redundancy – they could check for both properties but be fed input in those 3 separate conditions. It would also be easy to cover other limit cases too (e.g. empty input, only petenv, only a single non-petenv).

@Saviq,

I didn't see your edit where you added suggesting a third test covering the conjunction of the two.

I'm good with this whole testing approach 😁

ricab · 2019-08-26T16:51:54Z

tests/test_output_formatter.cpp

+    auto output = csv_formatter.format(list_reply);
+
+    EXPECT_EQ(output, expected_output);
+}


ricab · 2019-08-26T16:52:04Z

tests/test_output_formatter.cpp

+    auto output = formatter.format(list_reply);
+
+    EXPECT_EQ(output, expected_output);
+}


codecov · 2019-09-02T18:00:15Z

Codecov Report

Merging #994 into master will decrease coverage by 0.31%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
- Coverage   69.64%   69.32%   -0.32%     
==========================================
  Files         193      191       -2     
  Lines        7116     6984     -132     
==========================================
- Hits         4956     4842     -114     
+ Misses       2160     2142      -18

Impacted Files	Coverage Δ
include/multipass/cli/format_utils.h	`93.75% <83.33%> (-6.25%)`	⬇️
...orm/backends/shared/linux/qemuimg_process_spec.cpp	`0% <0%> (-100%)`	⬇️
include/multipass/ssh/ssh_process.h	`50% <0%> (-50%)`	⬇️
...rc/platform/backends/shared/linux/process_spec.cpp	`50% <0%> (-23.34%)`	⬇️
...rc/platform/backends/qemu/dnsmasq_process_spec.cpp	`90.47% <0%> (-3.08%)`	⬇️
src/platform/backends/qemu/dnsmasq_server.cpp	`90.9% <0%> (-1.4%)`	⬇️
...rc/platform/backends/qemu/qemu_vm_process_spec.cpp	`98.27% <0%> (-0.36%)`	⬇️
src/daemon/default_vm_image_vault.cpp	`74.92% <0%> (-0.08%)`	⬇️
src/daemon/daemon.cpp	`30.37% <0%> (-0.07%)`	⬇️
src/platform/backends/shared/linux/linux_process.h	`100% <0%> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37892a1...a02e204. Read the comment docs.

codecov · 2019-09-02T18:00:15Z

Codecov Report

Merging #994 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
+ Coverage   69.64%   69.66%   +0.01%     
==========================================
  Files         193      193              
  Lines        7116     7120       +4     
==========================================
+ Hits         4956     4960       +4     
  Misses       2160     2160

Impacted Files	Coverage Δ
include/multipass/cli/format_utils.h	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37892a1...b53a330. Read the comment docs.

Saviq · 2019-09-02T18:03:40Z

@ricab @townsend2010 this should be better then.

ricab

OK, I am good with this. I have just a tiny request, if you feel like it, and a question. I'll wait for your reply before approving.

ricab · 2019-09-04T10:01:03Z

tests/test_output_formatter.cpp

-class CSVFormatter : public LocaleSettingTest
-{
-};
+typedef std::tuple<const mp::Formatter*, const ::google::protobuf::Message*, std::string, std::string>


It's a bit difficult to track what each element means (here and in the petenv suite). Could we maybe have a comment to help the reader?

ricab · 2019-09-04T10:33:19Z

tests/test_output_formatter.cpp

+    const auto param = GetParam();
+    const auto formatter = std::get<0>(param);
+    const auto reply = std::get<1>(param);
+    const auto expected_output = std::get<2>(param);


Did you mean to copy param and then its contents? Not that it matters much, I suppose...

Moved to structured bindings now. Could be even nicer with [[maybe_unused]] and nesting, but not possible yet.

ricab

Thanks!

bors r+

994: [list] sort list reply alphabetically (Fixes #836) r=ricab a=Saviq As part of that, refactor the formatter test suite into a parametrized one. Co-authored-by: Michał Sawicz <michal@sawicz.net>

bors · 2019-09-04T12:53:54Z

Build succeeded

continuous-integration/travis-ci/push

Saviq requested a review from ricab August 26, 2019 13:31

ricab reviewed Aug 26, 2019

View reviewed changes

[list] sort list reply alphabetically (Fixes #836)

f6b6981

Saviq force-pushed the sort-table-list-output branch from 9f0a514 to a02e204 Compare September 2, 2019 18:00

Saviq added 7 commits September 2, 2019 20:07

[tests] parameterize TableFormatter list tests

50b5f0e

[tests] make ListFormatterSuite formatter-generic

297a443

[tests] make FormatterSuite reply-generic

dabb279

[tests] move table info tests to the FormatterSuite

287ca9c

[tests] move FindReply table formatting tests to FormatterSuite

2775f64

[tests] move JsonFormatter tests to FormatterSuite

976a079

[tests] move CSVFormatter tests to FormatterSuite

13a3861

Saviq force-pushed the sort-table-list-output branch 2 times, most recently from 8e91453 to 1bd5780 Compare September 2, 2019 18:12

[tests] move YamlFormatter tests to FormatterSuite

3c910fd

Saviq force-pushed the sort-table-list-output branch from 1bd5780 to 3c910fd Compare September 2, 2019 18:14

Saviq requested review from ricab and townsend2010 September 2, 2019 18:30

[tests] add prepend case in FormatterSuite

5770b11

ricab reviewed Sep 4, 2019

View reviewed changes

Saviq added 2 commits September 4, 2019 13:29

[tests] add comments explaining FormatterSuite parameters

42c329b

[tests] use structured bindings in FormatterSuite

b53a330

ricab approved these changes Sep 4, 2019

View reviewed changes

bors bot added a commit that referenced this pull request Sep 4, 2019

Merge #994

f0d335d

994: [list] sort list reply alphabetically (Fixes #836) r=ricab a=Saviq As part of that, refactor the formatter test suite into a parametrized one. Co-authored-by: Michał Sawicz <michal@sawicz.net>

bors bot merged commit b53a330 into master Sep 4, 2019

bors bot deleted the sort-table-list-output branch September 4, 2019 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[list] sort list reply alphabetically (Fixes #836) #994

[list] sort list reply alphabetically (Fixes #836) #994

Saviq commented Aug 26, 2019 •

edited

ricab left a comment

ricab Aug 26, 2019

Saviq Aug 27, 2019 •

edited

ricab Aug 27, 2019

townsend2010 Aug 27, 2019

Saviq Aug 27, 2019

ricab Aug 27, 2019 •

edited

townsend2010 Aug 28, 2019

ricab Aug 26, 2019

ricab Aug 26, 2019

codecov bot commented Sep 2, 2019

codecov bot commented Sep 2, 2019 •

edited

Saviq commented Sep 2, 2019

ricab left a comment

ricab Sep 4, 2019

Saviq Sep 4, 2019

ricab Sep 4, 2019

Saviq Sep 4, 2019

ricab left a comment

bors bot commented Sep 4, 2019

[list] sort list reply alphabetically (Fixes #836) #994

[list] sort list reply alphabetically (Fixes #836) #994

Conversation

Saviq commented Aug 26, 2019 • edited

ricab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saviq Aug 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricab Aug 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2019

Codecov Report

codecov bot commented Sep 2, 2019 • edited

Codecov Report

Saviq commented Sep 2, 2019

ricab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

bors bot commented Sep 4, 2019

Build succeeded

Saviq commented Aug 26, 2019 •

edited

Saviq Aug 27, 2019 •

edited

ricab Aug 27, 2019 •

edited

codecov bot commented Sep 2, 2019 •

edited