search

datawire / teleproxy

18 stars 9 forks source link

Adjust `testprocess.MakeSudo()` to not imply `dtest.Sudo()` #168

Closed LukeShu closed 5 years ago

LukeShu commented 5 years ago

Currently, testprocess.MakeSudo() implies dtest.Sudo(), which causes all of the test code for that package to run as root (except for code behind testprocess.Make(). That is a... weird and surprising implication. Get rid of it; if code wants the "main" test code to run as root, have it call dtest.Sudo() directly.

Also relevant: It had caused testprocess.Make() to spawn processes through a few layers of sudo; first sudo to root, then sudo -u $SUDO_USER back to the original user. It's a pointless step that can only screw things up (like the /etc/sudoers security policy overriding PATH). Now it only uses sudo when necessary.

ark3 commented 5 years ago

I like this. The only caveat with calling MakeSudo without using dtest.Sudo() (which should really be called RunAsRoot or something) is that you may get multiple sudo password prompts during your test run.

LukeShu commented 5 years ago

That is true. But under "normal" circumstances, I would expect the sudo token to be valid after the first one, so in practice you'd still only get one prompt?

ark3 commented 5 years ago

Depends on how long your tests take. If one test waits 5 minutes on the machine lock and then calls sudo, another test's earlier sudo won't help. I've experienced this by calling sudo true in the Edge Control tests, which isn't quite the same thing, but would be with the above. Perhaps we can count on tests being fast enough that this isn't a problem in practice. Or every test will just call dtest.Sudo() and that'll be that. Regardless, I like this.