| Message ID | 20180607223111.27792-2-ross.zwisler@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote: > Currently if "make check" detects a mismatch in the ASL generated during > testing, we print an error such as: > > acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl, > aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl, > aml:tests/acpi-test-data/q35/SSDT.dimmpxm]. > > but the testing still exits with good shell status. This is wrong, and > makes bisecting such a failure difficult. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Failing would also mean that any change must update the expected files at the same time. And that in turn is problematic because expected files are binary and can't be merged. In other words the way we devel ACPI right now means that bisect will periodically produce a diff, it's not an error. > --- > tests/bios-tables-test.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > index 256d463cb8..9b5db1ee8f 100644 > --- a/tests/bios-tables-test.c > +++ b/tests/bios-tables-test.c > @@ -469,6 +469,7 @@ static void test_acpi_asl(test_data *data) > } > } > } > + g_test_fail(); > } > g_string_free(asl, true); > g_string_free(exp_asl, true); > -- > 2.14.4
On 08.06.2018 01:09, Michael S. Tsirkin wrote: > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote: >> Currently if "make check" detects a mismatch in the ASL generated during >> testing, we print an error such as: >> >> acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl, >> aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl, >> aml:tests/acpi-test-data/q35/SSDT.dimmpxm]. >> >> but the testing still exits with good shell status. This is wrong, and >> makes bisecting such a failure difficult. >> >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > Failing would also mean that any change must update the expected files > at the same time. And that in turn is problematic because expected > files are binary and can't be merged. > > In other words the way we devel ACPI right now means that bisect will > periodically produce a diff, it's not an error. But apparently the current way also allows that real bug go unnoticed for a while, until somebody accidentially spots the warning in the output of "make check". Wouldn't it be better to fail at CI time already? If a merge of the file is required, you can still resolve that manually (i.e. by rebasing one of the pull requests). Thomas
On Fri, Jun 08, 2018 at 07:17:51AM +0200, Thomas Huth wrote: > On 08.06.2018 01:09, Michael S. Tsirkin wrote: > > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote: > >> Currently if "make check" detects a mismatch in the ASL generated during > >> testing, we print an error such as: > >> > >> acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl, > >> aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl, > >> aml:tests/acpi-test-data/q35/SSDT.dimmpxm]. > >> > >> but the testing still exits with good shell status. This is wrong, and > >> makes bisecting such a failure difficult. > >> > >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > Failing would also mean that any change must update the expected files > > at the same time. And that in turn is problematic because expected > > files are binary and can't be merged. > > > > In other words the way we devel ACPI right now means that bisect will > > periodically produce a diff, it's not an error. > > But apparently the current way also allows that real bug go unnoticed > for a while, until somebody accidentially spots the warning in the > output of "make check". Wouldn't it be better to fail at CI time > already? If a merge of the file is required, you can still resolve that > manually (i.e. by rebasing one of the pull requests). I share this point of view. The unit tests only add value if we keep them up to date and passing as we modify the source. The ACPI tables in this case were broken in an innocuous way and just needed to be updated to match again, but it means that the tests for them are now basically turned off. Someone else could come along and break the ACPI table in a real and harmful way, and nobody would notice because the and result would still just be an ACPI table mismatch that is non-fatal and ignored.
On Fri, Jun 08, 2018 at 09:34:02AM -0600, Ross Zwisler wrote: > On Fri, Jun 08, 2018 at 07:17:51AM +0200, Thomas Huth wrote: > > On 08.06.2018 01:09, Michael S. Tsirkin wrote: > > > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote: > > >> Currently if "make check" detects a mismatch in the ASL generated during > > >> testing, we print an error such as: > > >> > > >> acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl, > > >> aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl, > > >> aml:tests/acpi-test-data/q35/SSDT.dimmpxm]. > > >> > > >> but the testing still exits with good shell status. This is wrong, and > > >> makes bisecting such a failure difficult. > > >> > > >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > > > Failing would also mean that any change must update the expected files > > > at the same time. And that in turn is problematic because expected > > > files are binary and can't be merged. > > > > > > In other words the way we devel ACPI right now means that bisect will > > > periodically produce a diff, it's not an error. > > > > But apparently the current way also allows that real bug go unnoticed > > for a while, until somebody accidentially spots the warning in the > > output of "make check". Wouldn't it be better to fail at CI time > > already? If a merge of the file is required, you can still resolve that > > manually (i.e. by rebasing one of the pull requests). > > I share this point of view. The unit tests only add value if we keep them up > to date and passing as we modify the source. The ACPI tables in this case > were broken in an innocuous way and just needed to be updated to match again, > but it means that the tests for them are now basically turned off. The expected value tests are a debugging aid. They do not catch bugs and aren't designed to. In particular the comparisons do not even run if IASL isn't installed. > Someone > else could come along and break the ACPI table in a real and harmful way, and > nobody would notice because the and result would still just be an ACPI table > mismatch that is non-fatal and ignored. There are tests with windows and linux guests that will catch it. It's just not something we can handle reasonably in a unit test.
On Fri, Jun 08, 2018 at 07:17:51AM +0200, Thomas Huth wrote: > On 08.06.2018 01:09, Michael S. Tsirkin wrote: > > On Thu, Jun 07, 2018 at 04:31:08PM -0600, Ross Zwisler wrote: > >> Currently if "make check" detects a mismatch in the ASL generated during > >> testing, we print an error such as: > >> > >> acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl, > >> aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl, > >> aml:tests/acpi-test-data/q35/SSDT.dimmpxm]. > >> > >> but the testing still exits with good shell status. This is wrong, and > >> makes bisecting such a failure difficult. > >> > >> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > Failing would also mean that any change must update the expected files > > at the same time. And that in turn is problematic because expected > > files are binary and can't be merged. > > > > In other words the way we devel ACPI right now means that bisect will > > periodically produce a diff, it's not an error. > > But apparently the current way also allows that real bug go unnoticed > for a while, until somebody accidentially spots the warning in the > output of "make check". Wouldn't it be better to fail at CI time > already? If a merge of the file is required, you can still resolve that > manually (i.e. by rebasing one of the pull requests). > > Thomas Pull requests are somewhat different, they are usually tested for lack of warnings. This change didn't arrive as a result of a pull request maybe that's why it slipped through the cracks. Peter? Maybe we need a "pedantic" flag to fail on any warnings, or just catch output to stderr.
On 8 June 2018 at 16:59, Michael S. Tsirkin <mst@redhat.com> wrote: > The expected value tests are a debugging aid. They do not catch bugs and > aren't designed to. In particular the comparisons do not even run if > IASL isn't installed. If they're not actually tests to catch bugs, maybe we shouldn't be running them in "make check" ? thanks -- PMM
On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote: > Pull requests are somewhat different, they are usually tested for lack > of warnings. This change didn't arrive as a result of a pull request > maybe that's why it slipped through the cracks. Peter? > > Maybe we need a "pedantic" flag to fail on any warnings, or just catch > output to stderr. If there's a situation that shouldn't exist in the tree (ie a bug), then make check should catch it, and result in a failure, not just printing random stuff to stderr. Otherwise I'm not going to notice it, whether I'm applying a pull request or an individual patch. thanks -- PMM
On Fri, Jun 08, 2018 at 05:14:09PM +0100, Peter Maydell wrote: > On 8 June 2018 at 16:59, Michael S. Tsirkin <mst@redhat.com> wrote: > > The expected value tests are a debugging aid. They do not catch bugs and > > aren't designed to. In particular the comparisons do not even run if > > IASL isn't installed. > > If they're not actually tests to catch bugs, maybe we shouldn't > be running them in "make check" ? > > thanks > -- PMM We are running tests to do basic things like verifying the checksum of the tables. Failure on these causes make check to fail. As long as we have the tables anyway, and assuming IASL is installed (which most people who do not work on ACPI would not have) we compare to the expected set, that is often helpful for debugging.
On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote: > On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote: > > Pull requests are somewhat different, they are usually tested for lack > > of warnings. This change didn't arrive as a result of a pull request > > maybe that's why it slipped through the cracks. Peter? > > > > Maybe we need a "pedantic" flag to fail on any warnings, or just catch > > output to stderr. > > If there's a situation that shouldn't exist in the tree (ie > a bug), then make check should catch it, and result in a > failure, not just printing random stuff to stderr. Otherwise > I'm not going to notice it, whether I'm applying a pull request > or an individual patch. > > thanks > -- PMM It's ok if it happens, but it just makes debugging and reviewing ACPI patches a little bit harder until it's fixed.
On 08.06.2018 18:24, Michael S. Tsirkin wrote: > On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote: >> On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote: >>> Pull requests are somewhat different, they are usually tested for lack >>> of warnings. This change didn't arrive as a result of a pull request >>> maybe that's why it slipped through the cracks. Peter? >>> >>> Maybe we need a "pedantic" flag to fail on any warnings, or just catch >>> output to stderr. >> >> If there's a situation that shouldn't exist in the tree (ie >> a bug), then make check should catch it, and result in a >> failure, not just printing random stuff to stderr. Otherwise >> I'm not going to notice it, whether I'm applying a pull request >> or an individual patch. >> >> thanks >> -- PMM > > It's ok if it happens, but it just makes debugging and reviewing > ACPI patches a little bit harder until it's fixed. It's maybe ok for *you*, but this certainly confuses everybody else. If I want to check my patches and suddenly some strange warnings are popping up, I first assume that there is something wrong in my patches (since I assume that the git repository is clean by default). So I've got to waste my time debugging issues that are not my own. Thanks for that :-/ Thomas
On Fri, Jun 08, 2018 at 07:23:06PM +0200, Thomas Huth wrote: > On 08.06.2018 18:24, Michael S. Tsirkin wrote: > > On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote: > >> On 8 June 2018 at 17:03, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> Pull requests are somewhat different, they are usually tested for lack > >>> of warnings. This change didn't arrive as a result of a pull request > >>> maybe that's why it slipped through the cracks. Peter? > >>> > >>> Maybe we need a "pedantic" flag to fail on any warnings, or just catch > >>> output to stderr. > >> > >> If there's a situation that shouldn't exist in the tree (ie > >> a bug), then make check should catch it, and result in a > >> failure, not just printing random stuff to stderr. Otherwise > >> I'm not going to notice it, whether I'm applying a pull request > >> or an individual patch. > >> > >> thanks > >> -- PMM > > > > It's ok if it happens, but it just makes debugging and reviewing > > ACPI patches a little bit harder until it's fixed. > > It's maybe ok for *you*, but this certainly confuses everybody else. If > I want to check my patches and suddenly some strange warnings are > popping up, I first assume that there is something wrong in my patches > (since I assume that the git repository is clean by default). So I've > got to waste my time debugging issues that are not my own. Thanks for > that :-/ > > Thomas Right so normally these do not pop out at all as I fix expected with a patch on top.
On 08.06.2018 20:41, Michael S. Tsirkin wrote: > On Fri, Jun 08, 2018 at 07:23:06PM +0200, Thomas Huth wrote: >> On 08.06.2018 18:24, Michael S. Tsirkin wrote: >>> On Fri, Jun 08, 2018 at 05:16:30PM +0100, Peter Maydell wrote: [...] >>>> If there's a situation that shouldn't exist in the tree (ie >>>> a bug), then make check should catch it, and result in a >>>> failure, not just printing random stuff to stderr. Otherwise >>>> I'm not going to notice it, whether I'm applying a pull request >>>> or an individual patch. >>>> >>> It's ok if it happens, but it just makes debugging and reviewing >>> ACPI patches a little bit harder until it's fixed. >> >> It's maybe ok for *you*, but this certainly confuses everybody else. If >> I want to check my patches and suddenly some strange warnings are >> popping up, I first assume that there is something wrong in my patches >> (since I assume that the git repository is clean by default). So I've >> got to waste my time debugging issues that are not my own. Thanks for >> that :-/ > > Right so normally these do not pop out at all as I fix expected > with a patch on top. Apparently other people can also introduce changes that cause these warnings. Anyway, I now "fixed" it here by uninstalling iasl, so never mind. Thomas
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c index 256d463cb8..9b5db1ee8f 100644 --- a/tests/bios-tables-test.c +++ b/tests/bios-tables-test.c @@ -469,6 +469,7 @@ static void test_acpi_asl(test_data *data) } } } + g_test_fail(); } g_string_free(asl, true); g_string_free(exp_asl, true);
Currently if "make check" detects a mismatch in the ASL generated during testing, we print an error such as: acpi-test: Warning! SSDT mismatch. Actual [asl:/tmp/asl-QZDWJZ.dsl, aml:/tmp/aml-T8JYJZ], Expected [asl:/tmp/asl-DTWVJZ.dsl, aml:tests/acpi-test-data/q35/SSDT.dimmpxm]. but the testing still exits with good shell status. This is wrong, and makes bisecting such a failure difficult. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> --- tests/bios-tables-test.c | 1 + 1 file changed, 1 insertion(+)