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

[Ani Sinha]

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 :-)

>
> 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?

>
> 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
>