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

[Igor Mammedov]

On Sat, 19 Sep 2020 08:49:40 +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>
> ---
>  tests/qtest/bios-tables-test.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> changelog:
> v1: initial patch
> v2: cosmetic - commit log reworded.
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 3f7f1a8107..e51ea26ae8 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 tables:
s/tables/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
> @@ -48,6 +48,24 @@
>   * - patches 2 - n: real changes, may contain multiple patches.
>   * - patch n + 1: update golden master binaries and empty
>   *   tests/qtest/bios-tables-test-allowed-diff.h
> + *
> + * There is a reason why the above process is followed. After every commit we
> + * make sure that the unit tests are not broken.

> + * Listing changed files in patch 1 makes sure every commit that follows 
> which
> + * affect the tests (patches 2 - n) does not break tests.
this is already mentioned earlier:
"
 * After 1-3 above tests will pass but ignore differences with the expected 
files.
"

> + * This is followed by the actual changes (test changes or code changes) that
> + * actually affect the acpi tables.
> + * Finally in patch n + 1, we update the golden master blobs as well as 
> revert
> + * the file additions in bios-tables-test-allowed-diff.h. This makes sure 
> that
> + * the test continues to pass because of updated table blobs while the state 
> of
> + * bios-tables-test-allowed-diff.h is reverted back to the default empty file
> + * condition.
this is also already documented:
"
 * The resulting patchset/pull request then looks like this:                    
 
 * - patch 1: list changed files in 
tests/qtest/bios-tables-test-allowed-diff.h. 
 * - patches 2 - n: real changes, may contain multiple patches.                 
 
 * - patch n + 1: update golden master binaries and empty                       
           
 *   tests/qtest/bios-tables-test-allowed-diff.h
"

> + *
> + * We could have committed the table updates along with the patches. However,
> + * whereas a code change is easily reviewable and in case of conflicts, 
> easily
> + * addressible, a binary blob is not. Hence, its better to commmit the binary
> + * blob updates as a separate independent commit. Listing the modified table
> + * files additionally helps in bisection in case things are broken.
>   */
I'd suggest to move rationale to step 6 description.
something like this:
  "expected binary updates should 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"

>  
>  #include "qemu/osdep.h"