Merge ~emitorino/qa-regression-testing:test_daemon_update into qa-regression-testing:master

nagios and nagios2 were superceded by nagios3 and removed from the ubuntu archive a long time ago (publishing histories are https://launchpad.net/ubuntu/+source/nagios/+publishinghistory and https://launchpad.net/ubuntu/+source/nagios2/+publishinghistory resepctively). We can just kill the respective test scripts.

> nagios and nagios2 were superceded by nagios3 and removed from the ubuntu
> archive a long time ago (publishing histories are
> https://launchpad.net/ubuntu/+source/nagios/+publishinghistory and
> https://launchpad.net/ubuntu/+source/nagios2/+publishinghistory resepctively).
> We can just kill the respective test scripts.

Done!

Removing nginx from the list of failing tests as it is working properly.

Thanks so much for working on this!

puppet got dropped from main between trusty (14.04) and xenial (16.04) so I'm not at all surprised bitrot occurred with that test script.

vsftp I can reproduce test failures without this branch's changes, it looks like the temporary users may not be set up correctly, because I'm getting auth (503) errors in the testcase on xenial (only place I've tested).

I could not reproduce the nginx failure; change looks good to me there.

See comment on the postgresql test.

Still looking at other changes.

Reviewing the specific tests that are failing:

test-fuse.py: is not releated to this change; there's something goofy where sshfs/mount isn't updating /etc/mtab in some situations that's causing a couple of tests to fail.

test-libfs.py: I *think* this test is to verify the client library of xfs, the X font server... which was dropped during the trusty development cycle (so we need to keep the test around until the last stake has been driven into precise ESM). Any issue here does not block your changes, and in fact, on precise, the tests pass with these changes in place.

test-mailman: test requires a bit of setup during the install phase. One failure on bionic, not due to this change (but likely due to a recent apache update). Not a blocker.

test-rsync: failure is alas due to a known issue introduced by a glibc update, which upstream rsync has yet to address, and is unrelated to these changes. Not a blocker.

I'll look at the rest of the failing scripts tomorrow, but I'm having a hard time seeing how these fixes could be introducing issues beyond what's already been noted.

On the whole, the changes look great. I added some additional code comments, but I don't see anything additional that warrants blocking this from being merged.

I have added a new commit including your suggestions sbeattie. BUT, I run test-postgresql-8.4 on precise VM just in case but noticed tests fail there (even prior my changes, so adding this test to the list of failures).

On Thu, Sep 26, 2019 at 03:02:23PM -0000, Maria Emilia Torino wrote:
> I have added a new commit including your suggestions sbeattie. BUT,
> I run test-postgresql-8.4 on precise VM just in case but noticed tests
> fail there (even prior my changes, so adding this test to the list
> of failures).

Ack. With your updated branch, it looks like the postgresql-8.4 test
fails because the libecpg-dev package wasn't installed, so not related
to your changes.

> Diff comments:
>
> > class TestDaemon:
> > '''Helper class to manage daemons consistently'''
> > - def __init__(self, init):
> > + def __init__(self, service_name):
> > '''Setup daemon attributes'''
> > - self.initscript = init
> > + if os.path.isfile('/usr/sbin/service'):
>
> Well, I can add the verification just in case! Python has:
> os.path.islink('/usr/sbin/service'), so I can add the OR condition to
> this line. Sounds good?

That'd be fine, I think.

> > + self.test_daemon_commands = ['service', service_name]
> > + else:
> > + self.test_daemon_commands = ['/etc/init.d/' + service_name]
> >
> > - def start(self):
> > - '''Start daemon'''
> > - rc, report = cmd([self.initscript, 'start'])
> > + def _run_command(self, action):
> > + command = self.test_daemon_commands[:]
>
> I meant to have a single variable to identify the main command to run
> during init (cmd gets a list as argument), and just append the desired
> action when required.
>
> I had 2 options on running the command: either appending + removing to
> the original list on each run (so I always keep the original command
> clean). Or creating locally a new list (based on the original commands
> list). I took the 2nd approach.... just because seemed more clear to
> me, but I can go append + remove if preferred.
>
> So, taking the 2nd approach, the assignment operator (=) in Python
> makes the two variables point to the same list in memory. So even if I
> create a new list, the changes affect the original one as well. '[:]'
> creates a shallow copy instead: creates a new object and the original
> is not modified JUST in this case, because it does not have compound
> objects (otherwise are still referenced). deepcopy is the 3rd option,
> where elements have their own pointers, but it is slower and not needed
> in this case

Thanks for the detailed explanation. Using [:] for a
list copy is a python idiom I was unfamiliar with. Could
we use .copy() or list() instead, to make it more explicit for
jetlagged-programmer-flailing-about-to-figure-out-why-their-tests-are-failing
comprehension?

(Given the small size of the list and the fact that we're sprinkling
multi-second sleeps around, performance of one implementation over
another should take a backseat to developer comprehension IMO.)

> This article is nice for understanding
> the differences when copying lists:
> https://medium.com/@thawsitt/assignment-vs-shallow-copy-vs-deep-copy-in-python-f70c2f0ebd86.

>
> > + command.append(action)
> > + rc, report = cmd(command)
> > expected = 0
> > result = 'Got exit code %d, expected %d\n' % (rc, expected)
> > - ...

> On Thu, Sep 26, 2019 at 03:02:23PM -0000, Maria Emilia Torino wrote:
> > I have added a new commit including your suggestions sbeattie. BUT,
> > I run test-postgresql-8.4 on precise VM just in case but noticed tests
> > fail there (even prior my changes, so adding this test to the list
> > of failures).
>
> Ack. With your updated branch, it looks like the postgresql-8.4 test
> fails because the libecpg-dev package wasn't installed, so not related
> to your changes.

Confirmed that the tests pass with the libecpg-dev package installed. Fix pushed to qrt master.

Thanks!

> On Thu, Sep 26, 2019 at 03:02:23PM -0000, Maria Emilia Torino wrote:
> > I have added a new commit including your suggestions sbeattie. BUT,
> > I run test-postgresql-8.4 on precise VM just in case but noticed tests
> > fail there (even prior my changes, so adding this test to the list
> > of failures).
>
> Ack. With your updated branch, it looks like the postgresql-8.4 test
> fails because the libecpg-dev package wasn't installed, so not related
> to your changes.
>

Great! I still have issues on my precise VM, but if you got them working, I think we can proceed with the merge. Can you try to run it with my branch just in case please? or run it once I merge it? :)

> > Diff comments:
> >
> > > class TestDaemon:
> > > '''Helper class to manage daemons consistently'''
> > > - def __init__(self, init):
> > > + def __init__(self, service_name):
> > > '''Setup daemon attributes'''
> > > - self.initscript = init
> > > + if os.path.isfile('/usr/sbin/service'):
> >
> > Well, I can add the verification just in case! Python has:
> > os.path.islink('/usr/sbin/service'), so I can add the OR condition to
> > this line. Sounds good?
>
> That'd be fine, I think.
>
> > > + self.test_daemon_commands = ['service', service_name]
> > > + else:
> > > + self.test_daemon_commands = ['/etc/init.d/' + service_name]
> > >
> > > - def start(self):
> > > - '''Start daemon'''
> > > - rc, report = cmd([self.initscript, 'start'])
> > > + def _run_command(self, action):
> > > + command = self.test_daemon_commands[:]
> >
> > I meant to have a single variable to identify the main command to run
> > during init (cmd gets a list as argument), and just append the desired
> > action when required.
> >
> > I had 2 options on running the command: either appending + removing to
> > the original list on each run (so I always keep the original command
> > clean). Or creating locally a new list (based on the original commands
> > list). I took the 2nd approach.... just because seemed more clear to
> > me, but I can go append + remove if preferred.
> >
> > So, taking the 2nd approach, the assignment operator (=) in Python
> > makes the two variables point to the same list in memory. So even if I
> > create a new list, the changes affect the original one as well. '[:]'
> > creates a shallow copy instead: creates a new object and the original
> > is not modified JUST in this case, because it does not have compound
> > objects (otherwise are still referenced). deepcopy is the 3rd option,
> > where elements have their own pointers, but it is slower and not needed
> > in this case
>
> Thanks for the detailed explanation. Using [:] for a
> list copy is a python idiom I was unfamiliar with. Could
> we use .copy() or list() instead, to make it more explicit for
> jetlagged-programmer-flailing-about-to-figure-out-why-their-tests-are-failing
> comprehension?
>
> (Given the small size of the list and the fact that we're sprinkling
> multi-second sleeps around, performance of one implementation over
> another should take a backseat to developer comprehension IMO.)

Of course! Modified to list(), as ....

Yep, re-ran postgresql-8.4 install and test on precise and worked for me.

(I am curious how it's failing for you, can you paste the test results somewhere?)

I investigated test-mono-xsp; it needs to be updated to take into account xsp2 vs xsp4, which is out of scope for this pr. Same with the other failing tests that I've investigated.

I'm happy with everything here. Please merge.

Thanks again, especially for being patient with me!