On Tue, 22 Sep 2020 17:06:42 +0530 Ani Sinha <a...@anisinha.ca> wrote:
> On Tue, Sep 22, 2020 at 4:57 PM Igor Mammedov <imamm...@redhat.com> wrote: > > > > On Tue, 22 Sep 2020 15:39:18 +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> > > > > 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 <imamm...@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 > > >
