[avocado] stageone test query on assertFileCollected

[arif-ali]

Now, that the dust is settling with converting the tests folder for both flake8 and pylint (a few PRs still outstanding). I have started to look on adding some tests, and I was a bit curious on the assertFileCollected function

    def assertFileCollected(self, fname):
        """Ensure that a given fname is in the extracted archive if it exists
        on the host system

        :param fname:  The name of the file within the archive
        :type fname:  ``str``
        """
        if fname.startswith(('sos_', '/sos_')) or os.path.exists(fname):
            self.assertFileExists(self.get_name_in_archive(fname))
        else:
            assert True

Now, in one of the code pieces I am writing, I added the following to the tests/report_tests/plugin_tests/networking.py

   @ubuntu_only
   def test_netplan_config_collected(self):
       self.assertFileCollected(
           '/etc/netplan/90-NM-d377ae8e-fff5-11ee-bd96-07ef2a5f9e02.yaml')

however the file was not actually collected by the sosreport, but the stageone tests still passes. Looking at the code, to me this seems like, if the file does not exist, it will just PASS. It would be good clarify on this, as both me and David are planning to write many more tests, but want to make sure we do this correctly.

At the moment, to me, this test just seems a bit redundant if the file doesn't exist. Shouldn't we assert False here?

[TurboTurtle]

Yes, assertFileCollected() returns True if the file is missing in the archive when the source file is missing from the host. This is to reflect the behavior of add_copy_spec() - if the given file path doesn't exist, we don't report an error or fail in some way. So this assertion was written to not fail the entire test suite when we hit a test system that doesn't have a particular file available for sos to collect.

Definitely open to hearing alternative approaches.

[arif-ali]

I just see the test as redundant and a pointless test, as we could add anything and everything in the testing, and it wouldn't make a difference.

imho, we should have the testing in this scenario, to ensure that we are collecting the file, and if it isn't there, then mock it such that it would be there, so that sos would collect it; that would then give us a true test to see if our plugins are collecting the write thing.

I think, it shouldn't reflect the behaviour of add_copy_spec(), rather we are asserting that we are collecting that file.

At least those are my thoughts on this; otherwise we can create 100s of tests, which wouldn't be meaningful. Even my example above is actually wrong, as the UUID of that file could be anything, and this would be better in stagetwo testing instead, and have an example file to copy over.

EDIT:

I can see that we add these, and then we can run these tests on our test environments and we can verify relevant bits, so I can see it from that perspective

[pmoravec]

I think it depends what do you want to test.

If the objective is "ensure the add_copy_spec behaved as expected for this file", then this test shall be used (maybe renamed, then, as it does not technically test if file was (unconditionally) collected, despite its name).

If the objective of the test is to verify some specific file (existing on the system or mocked with specific content) is collected every time (e.g. under some scenario like --keep-binary-files), then the method should test presence of the file in the archive unconditionally.

I see both methods make sense (though not having an example of particular test in hand), so worth renaming this method to something else, and having the assertFileCollected() one for unconditional test?

[TurboTurtle]

Yeah, I'd be onboard with renaming the current assertFileCollected() and creating some new replacement that makes it a hard requirement that the file be present regardless of it the source has it. The original design went with that reflection due to the myriad distros and distro versions we support, and not feeling super comfortable with a CI test suite that could fail quite so easily when our collection specifications are very broad by convention, specifically because we don't necessarily care if the file we want to collect is actually available to collect or not in actual report runs.

[arif-ali]

I can see both sides, and when I originally started on this earlier in the week, I thought it was a hard assert.

I think, it makes sense to have the soft assert, and actually thinking about it, this would actually help us to test everything for our release process and we don't have to do things manually, so all good from that perspective.

I do think, we ought to have a hard requirement too, as we would expect those files to be there regardless based on either a package installation, or there by default on the OS itself.