Re: UnitTest using the /tmp file system

[Sunitha Harish <sunithaharish04@gmail.com>]

On 27-03-2021 19:33, Patrick Williams wrote:
> 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
Thank you Patrick for the detailed view on this. Other repos using the 
/tmp for unit-test can be justified with this.

@Ed/Gunnar - can you please share your views as well? Currently bmcweb 
does not have the unit-test for methods which use dbus calls & which 
takes files as argument/return object. We may need some temporary test 
file in the unit-test setup. What will be the plan for bmcweb to handle 
these cases?

Thanks & regards,
Sunitha