| Message ID | 20200919031941.2664-1-ani@anisinha.ca |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Add a comment in bios-tables-test.c to clarify the reason behind approach | expand |
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"
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: * 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 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. + * + * 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. */ #include "qemu/osdep.h"
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.