Ensure verdi group show --limit respects limit even in raw mode
#4092
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4092 +/- ##
===========================================
- Coverage 78.81% 78.79% -0.01%
===========================================
Files 463 463
Lines 34413 34413
===========================================
- Hits 27119 27114 -5
- Misses 7294 7299 +5
Continue to review full report at Codecov.
|
The code is affected by the `--limit` flag both where the `--raw` flag is enabled and if it is disabled. However, in the disabled case the limit was not respected. This was not detected because the unit test only tested the code path when `--raw` is enabled.
sphuber
force-pushed
the
fix/4091/verdi-group-show-limit
branch
from
82ecf9f
to
dab183a
Compare
| self.assertTrue(not (str(nodes[0].pk) in result.output and str(nodes[1].pk) in result.output)) | ||
|
|
There was a problem hiding this comment.
Perhaps the second assert could be replaced by checking the length of the output to be 1? Feels a bit more straighforward and easy to read than the negative of the intersection (although less logically-fancy). Also it feels a bit more resistant to the DB leackeages that sometimes ocurr with the tests. Idk, just an idea, up to you.
There was a problem hiding this comment.
I would agree that that would be easier, except, unfortunately, in this case the output is not a list. It is a single string. That is the reason I used --raw before in the original test, so I would only get the nodes as rows, so I could exactly use the count. But that caused the hiding of the bug that this very PR resolves. So I cannot use --raw and so I have to "parse" the full string, or at best the lines of the result. Since I don't want to make the parsing dependent on the number of header and footer lines, I solved it like this.
There was a problem hiding this comment.
Uhm, I see, I didn't notice the string casting. Question: although I guess very unlikely, but this way of checking with strings could raise a false fail if the nodes were assigned, say, pk=1 and pk=12, because both "12" and "1" are in "12", correct?
There was a problem hiding this comment.
Well they should get adjacent identifiers, but that is besides the point. You are correct that this test is vulnerable to false positives, but also to false negatives.
Fixes #4091
The code is affected by the
--limitflag both where the--rawflagis enabled and if it is disabled. However, in the disabled case the
limit was not respected. This was not detected because the unit test
only tested the code path when
--rawis enabled.