Re: [Qemu-devel] [qemu PATCH 2/5] acpi: "make check" should fail on asl mismatch

[Ross Zwisler]

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.  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.