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

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

Ani Sinha posted 1 patch 3 years, 7 months ago

Diff against v2 v3 v4 v5
Download mbox

Test docker-quick@centos7 failed

Test docker-mingw@fedora failed

Test checkpatch failed

Test FreeBSD failed

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200918101441.16395-1-ani@anisinha.ca

Maintainers: Igor Mammedov <imammedo@redhat.com>, Thomas Huth <thuth@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>

There is a newer version of this series

tests/qtest/bios-tables-test.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)

Expand all Fold all

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

Posted by Ani Sinha 3 years, 7 months ago

A comment blob is added in bios-tables-test.c that explains 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(-)

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