Integration test for lp-1813396

[TheRealFalcon]

Proposed Commit Message

Integration test for lp-1813396 and #669

Ensure gpg is called with no tty flag.
Also, refactored the "ordered_items_in_text" to assert if the line
is missing and provide a more useful error message.

Additional Context

#669

Test Steps

fail:
CLOUD_INIT_CLOUD_INIT_SOURCE=NONE pytest tests/integration_tests/bugs/test_lp1813396.py
pass:
CLOUD_INIT_CLOUD_INIT_SOURCE=PROPOSED pytest tests/integration_tests/bugs/test_lp1813396.py

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[OddBloke]

This isn't what we're doing: we're testing that cloud-init passes --no-tty to gpg. I think that's sufficient for validation on Ubuntu (particularly as lxd doesn't support not providing /dev/tty to a system: https://lxd.readthedocs.io/en/latest/instances/#devices-configuration), but there could be an integration test for this bug which masks/removes /dev/tty somehow and then tests that we successfully configure the repo.

[OddBloke]

Are we better using a PPA for this? Easier to access when we're running tests inside our network, and we don't introduce a dependency on an "external" service.

[OddBloke]

This could also use a better failure message: now that we're using it in two places, perhaps ordered_items_in_text should return something (the first unfound item?) that could be used for a richer message? Or it could become assert_ordered_items_in_text and be responsible for raising an AssertionError with a richer message?

(A nit for this PR, but I would like to see this improved.)

[OddBloke]

On Wed, Dec 09, 2020 at 03:27:17PM -0800, James Falcon wrote: I guess I'm wondering what a different error message will buy us here. If it doesn't pass, we'll need to manually inspect the log file to see why, and I don't think a different error will change that.

That's a fair question. If someone is specifically running a test which uses this helper, then it doesn't buy us much, I agree. (And, I also agree that the usage _here_ specifically wouldn't benefit hugely from an improved message: all of these messages are expected to be very close together in the log; hence it being a nit. :) However, most of the time we're going to be running tests using this helper as part of a larger run. In that case, a clearer assertion message makes failures in "unknown" tests much easier to understand and deal with. It also makes failures easier to differentiate from one another: this is really asserting a bunch of things (all these items are in the log, item 1 < item 2, item 2 < item 3, etc.), and each of those different failures could indicate a different underlying issue that the test is revealing. Being able to compare these failures across test runs without debugging is valuable, and a message could draw a developer's attention to inconsistencies across releases (e.g. if xenial is failing at "Running command ..." but everything else is failing at "finish: modules-config ..." then we likely have two distinct issues, and you don't have to launch an instance for every single Ubuntu release to start narrowing things down).

we'll need to manually inspect the log file

This helper is not constrained to use on log files; it could well be used to inspect a string which is only in memory; once the test is over, this message is all the record we'll have of why it wasn't correct. (The same also applies if we have a test which is only failing in CI: we currently don't collect or store logs in Travis.)

[OddBloke]

Thanks! I've confirmed that this fails/passes as expected, and the change to the power state test (with my suggestion applied) also continues to behave as expected.