Enable skipped tests and fix dash dash parse bug#471
Conversation
ben-grande
force-pushed
the
fix-old-run-dash
branch
from
2cbd473 to
4e96de4
Compare
| with subprocess.Popen( | ||
| ["echo", "some-data"], stdout=subprocess.PIPE | ||
| ) as echo: | ||
| with unittest.mock.patch("sys.stdin", echo.stdout): |
There was a problem hiding this comment.
Why remove this? Cause later its already testing is stdin, stdout and stderr are None.
There was a problem hiding this comment.
Reverted this, not sure if I should touch that in this PR or commit. Maybe call a subprocess for shell and pass stdin that way, and capture stdout and stderr also that way?
Argparse interprets "--" as end of options separator, even if it already is inside nargs. Fix it by another less ugly hack that correctly distinguishes the dash dash when it's part of the command. Fixes: QubesOS/qubes-issues#2916
ben-grande
force-pushed
the
fix-old-run-dash
branch
from
4e96de4 to
5748465
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #471 +/- ##
=======================================
Coverage 76.83% 76.84%
=======================================
Files 53 53
Lines 9365 9363 -2
=======================================
- Hits 7196 7195 -1
+ Misses 2169 2168 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ( | ||
| "@dispvm:test-vm", | ||
| "qubes.VMExec+----", | ||
| "qubes.VMExec+----+----", |
There was a problem hiding this comment.
I don't think this is correct. -- (the first one) means end of options, it shouldn't be considered part of the command itself.
There was a problem hiding this comment.
In this case, yes. Old one was correct.
| ( | ||
| "@dispvm:test-vm", | ||
| "qubes.VMExec+command+----", | ||
| "qubes.VMExec+command+----+----", |
There was a problem hiding this comment.
And here too, the old behavior was correct.
There was a problem hiding this comment.
Command: ["--no-gui", "--dispvm=test-vm", "command", "--", "--"].
So when the command command is already the first positional argument, everything else should be passed to the VMExec and not be trimmed by qvm-run.
There was a problem hiding this comment.
-- is "end of options" marker, not "start of positional arguments" marker. It only means options shouldn't be interpreted after it, nothing more. See POSIX spec (Guideline 10).
See for example behavior of ls:
ls - l foo -- barls foo -l -- -- barls foo -- bar -l
That said, qvm-run already enforces all options to be passed before the command (for example qvm-run VM ls -p is not the same as qvm-run -p VM ls). So, maybe indeed in this case your approach is more consistent.
| self.app.expected_calls[ | ||
| ('test-vm', 'admin.vm.feature.CheckWithTemplate', 'os', None) | ||
| ] = b"2\x00QubesFeatureNotFoundError\x00\x00Feature 'os' not set\x00" | ||
| stdout = io.StringIO() |
There was a problem hiding this comment.
This is unused after your change (the assert at the end is pointless with the mock.patch removed).
2 participants
Argparse interprets "--" as end of options separator, even if it already is inside nargs. Fix it by another less ugly hack that correctly distinguishes the dash dash when it's part of the command.
Fixes: QubesOS/qubes-issues#2916
Didn't test on my system. Relying on unit tests.