UnitTest using the /tmp file system

[Patrick Williams]

On Sun, Mar 21, 2021 at 09:30:30AM +0530, Sunitha Harish wrote:

> This is regarding the unit tests in various repos under openbmc like: 
> phosphor-bmc-code-management, phosphor-logging, phosphor-networkd, pldm 
> etc . I have seen the testcases using the /tmp filesystem to create the 
> directories/files when the UT is run.
> 
> I followed the similar way of writing the UT in one of my commits 
> https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/37352 .  As per 
> the review comments in this commit, using the /tmp file system for UT is 
> anti-pattern, and this needs to be changed by mocking the same. I agree 
> that this is a valid thing to do.

With respect to this being an "anti-pattern", I'm not sure where this
statement is coming from.  Generally we have very few statements on how
unit-testing should be done across the project and this is certainly not
an "anti-pattern" documented in our usual place[1].

In terms of "what is appropriate to do in a unit test", I see two
possible perspectives:
    1. Anything is permissible and appropriate to do in a unit test (and
       thus nothing is *required* to be mocked).
    2. No system calls may be called from the unit test (and thus all
       system calls are required to be mocked).

Anything other than these two possibilities seems, to me, relatively
arbitrary without specific evidence that the un-mocked code is
problematic.  Nobody actually follows #2 because if you did you'd need
to mock even the 'sbrk' call which is used to create your heap.  So, the
question is why is one set of system calls reasonable to use in a
unit-test and another not?

We have many unit tests across the project that interact with either
the file system or dbus.  Whoever wrote them probably decided that was
the most pragmatic way to test their code and gain the coverage they
were looking for and whoever maintained the repository accepted that as
a solution.  In this case, I would encourage you to dig deeper into the
maintainer's opinion as to why this is an inappropriate approach for
this particular repository or unit-test.

There is one specific problem area we have encountered with fs-using UTs
and it is aggravated by the fact that we run tests in parallel and
sometimes Jenkins jobs land on the same machine: any files you create in
the file system, or dbus services you create, should use some sort of
randomness to avoid collisions between separate UTs.  I see in your
original commit, before the UTs were removed, that you used 'mkdtemp'
with an XXXXXX pattern which should resolve this potential issue.

1. https://github.com/openbmc/docs/blob/0f6c884822ca2d101e2a0bf3256ecf4f6f2431a3/anti-patterns.md

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20210327/db3031ef/attachment.sig>