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

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

[Ani Sinha]

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>
---
 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.
+ *    However, expected binary updates needs to be a separate patch from the
+ *    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.
  * 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
-- 
2.17.1

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

[Igor Mammedov]

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.

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

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

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
>

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