Enable some old expectedFailures for qvm_run#471
Conversation
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?
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.
There was a problem hiding this comment.
This is hard to do with argparse. It eats "--" anywhere even though option arguments are not allowed anymore at the place "--" is, because a preceding positional argument exists. Marking this as draft, but probably won't be solved.
| 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).
|
The dash-dash handling is haunted, I will drop the changes related to that so only the normal skipped tests can be enabled again and the issue can be closed. |
5748465 to
cf1ada1
Compare
ben-grande
changed the title
2 participants
For: QubesOS/qubes-issues#2916
Didn't test on my system. Relying on unit tests.