On Sat, 19 Sep 2020 08:49:40 +0530
Ani Sinha <a...@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 <a...@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"
