From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37018) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxC1n-00037U-H0 for qemu-devel@nongnu.org; Thu, 20 Oct 2016 08:00:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxC1l-0003eY-GR for qemu-devel@nongnu.org; Thu, 20 Oct 2016 08:00:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36792) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxC1l-0003e0-Bf for qemu-devel@nongnu.org; Thu, 20 Oct 2016 08:00:53 -0400 Date: Thu, 20 Oct 2016 13:00:49 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161020120048.GH2039@work-vm> References: <20161018105724.26520-1-pasic@linux.vnet.ibm.com> <20161018105724.26520-3-pasic@linux.vnet.ibm.com> <20161018132728.GH2190@work-vm> <20161018135426.GJ2190@work-vm> <20161018183228.GA22395@work-vm> <6e59d1a0-ec0c-2f23-e3d2-4c7a77c2b690@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6e59d1a0-ec0c-2f23-e3d2-4c7a77c2b690@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Halil Pasic Cc: Amit Shah , qemu-devel@nongnu.org, "quintela@redhat.com Guenther Hutzl" * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 10/18/2016 08:32 PM, Dr. David Alan Gilbert wrote: > >> > "The idea is to remove .start support and this patch should > >> > be reverted, as soon this happens, or even better just > >> > dropped. If however dropping the support for .start encounters > >> > resistance, this patch should prove useful in an unexpected > >> > way." > >> > > >> > the patch is not intended for a merge. My preferred way of dealing > >> > with this is to just pick (merge) the first and the last patch of the > >> > series. The second patch is just to prove that we have a problem, > >> > and it's effect is immediately reverted by the third patch as a > >> > preparation for the forth one which removes the tested feature altogether. > >> > > >> > In my opinion the inclusion of a commented out test makes even less > >> > sense if the tested feature is intended to be removed by the next > >> > patch in the series. > >> > > >> > I think I was not clear enough when stating that this patch is > >> > not intended for merging. Is there an established way to do > >> > this? > > I don't think there's any point in posting it like that as part > > of a patch series; posting it as a separate test that fails or > > something like that; but I don't think I've ever seen it done > > like that inside a patch series where you expect some of it > > to be picked up. > > > > Dave > > > > I understand. I assumed cherry-picking the two relevant patches from the > series would not be a problem here. I was wrong. > > Next time I will make sure to either do a separate failing test patch > and and cross reference in the cover letters, or to first do the fix and > then improve the test coverage so the bug does not come back. > > Should I send a v2 with the two questionable patches (the failing test > and the revert of it) removed right away? Yes, probably the best way; you can add the Review-by's I've just sent. Dave > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK