benhoyt
commented
6 months ago If it really becomes a concern later on due to more complex embedded scripts in test code, we could find a way to annotate embedded bash scripts in a consistent way that could allow a CI to pick them out in source files and shellcheck them.
Agreed, thanks.
Partly due to the fact that Pebble acts as a child subreaper, running additional commands outside the reaper is problematic. The "withReaper" argument to FakeCommand is a case in point.
However, instead of refactoring tests and FakeCommand to be reaper-aware, let's just remove the "maybe shellcheck" thing altogether.
I think it's problematic to have tests maybe do something if a specific command ("shellcheck" in this case) is installed on the system: some developers will catch issues with shellcheck, others won't. Plus, this has never caught anything for me in my Pebble journeys.
It also adds about 1s to the test running time, though that's not the main reason I want to take it out.
For reference, here is the original commit where it was added to snapd.