[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch |
Date: |
Fri, 8 Jun 2018 18:59:19 +0300 |
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 <address@hidden>
> > >
> > > 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.
--
MST
- [Qemu-devel] [qemu PATCH 1/5] gitignore: ignore generated qapi job files, Ross Zwisler, 2018/06/07
- [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Ross Zwisler, 2018/06/07
- Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Michael S. Tsirkin, 2018/06/08
- Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Peter Maydell, 2018/06/08
- Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Michael S. Tsirkin, 2018/06/08
- Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Thomas Huth, 2018/06/08
- Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Michael S. Tsirkin, 2018/06/08
- Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch, Thomas Huth, 2018/06/08
[Qemu-devel] [qemu PATCH 3/5] hw/i386: Update SSDT table used by "make check", Ross Zwisler, 2018/06/07