UnitTest using the /tmp file system

UnitTest using the /tmp file system

[Sunitha Harish]

Hi,

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.

Now this email is to discuss why this was originally done? Can the 
community come-up with a generalized solution for this ?

Thanks & regards,
Sunitha

Re: UnitTest using the /tmp file system

[Sunitha Harish]

Any views please ?

On 21-03-2021 09:30, Sunitha Harish wrote:
> Hi,
>
> 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.
>
> Now this email is to discuss why this was originally done? Can the 
> community come-up with a generalized solution for this ?
>
> Thanks & regards,
> Sunitha
>
>
>
>

Re: UnitTest using the /tmp file system

[Joseph Reynolds]

On 3/23/21 12:44 AM, Sunitha Harish wrote:
> Any views please ?
>
> On 21-03-2021 09:30, Sunitha Harish wrote:
>> Hi,
>>
>> 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.

Are the files written to the BMC's file system or to the test platform's 
file system?  I don't understand why this is important either way.  
Can't we just erase the file when the unit test is complete?

Joseph

>>
>> 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.
>>
>> Now this email is to discuss why this was originally done? Can the 
>> community come-up with a generalized solution for this ?
>>
>> Thanks & regards,
>> Sunitha
>>
>>
>>
>>

Re: UnitTest using the /tmp file system

[Lei Yu]

On Tue, Mar 23, 2021 at 11:08 PM Joseph Reynolds <jrey@linux.ibm.com> wrote:
>
> On 3/23/21 12:44 AM, Sunitha Harish wrote:
> > Any views please ?
> >
> > On 21-03-2021 09:30, Sunitha Harish wrote:
> >> Hi,
> >>
> >> 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.
>
> Are the files written to the BMC's file system or to the test platform's
> file system?  I don't understand why this is important either way.
> Can't we just erase the file when the unit test is complete?
>

I agree it is better to mock as many interfaces as possible to avoid
accessing host's file system.

But in certain cases, I do want to test the functions that involves filesystem.
In such case, I usually use mkdtemp() to create a temp dir in test
case's ctor or setup(), and remove it on test case's dtor or
tearDown().

-- 
BRs,
Lei YU

Re: UnitTest using the /tmp file system

[Sunitha Harish]

On 24-03-2021 08:42, Lei Yu wrote:
> On Tue, Mar 23, 2021 at 11:08 PM Joseph Reynolds <jrey@linux.ibm.com> wrote:
>> On 3/23/21 12:44 AM, Sunitha Harish wrote:
>>> Any views please ?
>>>
>>> On 21-03-2021 09:30, Sunitha Harish wrote:
>>>> Hi,
>>>>
>>>> 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.
>> Are the files written to the BMC's file system or to the test platform's
>> file system?  I don't understand why this is important either way.
>> Can't we just erase the file when the unit test is complete?
>>
> I agree it is better to mock as many interfaces as possible to avoid
> accessing host's file system.
>
> But in certain cases, I do want to test the functions that involves filesystem.
> In such case, I usually use mkdtemp() to create a temp dir in test
> case's ctor or setup(), and remove it on test case's dtor or
> tearDown().

Yes. The test platform's /tmp directory is used to create the files. The 
implementations in various repos are as Lei mentioned. The files are 
removed when the test is complete.

Re: UnitTest using the /tmp file system

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2753 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: UnitTest using the /tmp file system

[Sunitha Harish]

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