Re: [PATCH v3] Add a comment in bios-tables-test.c to clarify the reason behind approach

[Igor Mammedov]

On Tue, 22 Sep 2020 17:06:42 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> On Tue, Sep 22, 2020 at 4:57 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Tue, 22 Sep 2020 15:39:18 +0530
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > A comment blob is added in bios-tables-test.c that explains the reasoning
> > > behind the process of updating the ACPI table blobs when new tests are 
> > > added
> > > or old tests are modified or code is committed that affect tests. The
> > > explanation would help future contributors follow the correct process when
> > > making code changes that affect ACPI tables.
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>  
> >
> > My English is not good enough for writing documentation.
> > Maybe Eric will have something to suggest.
> >  
> > > ---
> > >  tests/qtest/bios-tables-test.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > Changelog:
> > > v2: cosmetic - commit log reworded.
> > > v3: review feedback incorporared and actual comment in the code reworded.
> > >
> > > diff --git a/tests/qtest/bios-tables-test.c 
> > > b/tests/qtest/bios-tables-test.c
> > > index b514b70b62..1e8838c8c6 100644
> > > --- a/tests/qtest/bios-tables-test.c
> > > +++ b/tests/qtest/bios-tables-test.c
> > > @@ -11,7 +11,7 @@
> > >   */
> > >
> > >  /*
> > > - * How to add or update the tests:
> > > + * How to add or update the tests or commit changes that affect ACPI 
> > > tables:
> > >   * Contributor:
> > >   * 1. add empty files for new tables, if any, under tests/data/acpi
> > >   * 2. list any changed files in 
> > > tests/qtest/bios-tables-test-allowed-diff.h
> > > @@ -38,6 +38,17 @@
> > >   *      $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh
> > >   * 6. Now commit any changes to the expected binary, include diff from 
> > > step 4
> > >   *    in commit log.
> > > + *    The reason why we follow the above process is described below:
> > > + *    After every commit we make sure that the unit tests are not 
> > > broken. To do
> > > + *    that, we could have committed the table updates along with the 
> > > patches.  
> > I'd drop upto here, it's obvious that that no patch should break build 
> > intentionally.  
> 
> I prefer to make this explicit. Build breakage != unit test breakage
> all the time :-)
when it comes to merge, broken test == broken build,
in addition I don't like negative example.
what we are missing currently is answer to "why" #is there,
and that's is answered by below the rest is just distraction to me.


> > Writing docs is hard, try to minimize amount of not really needed
> > information.
> >  
> > > + *    However, expected binary updates needs to be a separate patch from 
> > > the  
> > s/However,//
> >  
> > > + *    code that introduces changes to ACPI tables. It lets maintainer to 
> > > drop
> > > + *    and regenerate binary updates in case of merge conflicts. Further, 
> > > a code
> > > + *    change is easily reviewable but a binary blob is not (without 
> > > doing a
> > > + *    diassemly).  
> >  
> > > + * Listing the modified table files additionally helps in

> > > + *    bisection in case things are broken.
> > > + *    Hence, we have a multi-step process and at each step unit tests 
> > > continue
> > > + *    to pass.  
> > I'd drop this part as well  
> Why?

It doesn't belong to #6 and most of it is just one of the possible
justifications we can make up.
#1 is about turning hard error into warning to let test pass while
code being changed.
Try to concise and do not add extra sentences unless you have to.

> 
> >
> > with above corrections, I'm more or less fine with text so on that condition
> > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > the rest is upto Michael.
> >  
> > >   * 7. Before sending patches to the list (Contributor)
> > >   *    or before doing a pull request (Maintainer), make sure
> > >   *    tests/qtest/bios-tables-test-allowed-diff.h is empty - this will 
> > > ensure  
> >  
>