On Tue, Dec 29, 2020 at 10:17:31 -0600, Ryan Gahagan wrote: > On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa <pkrempa@xxxxxxxxxx> wrote: > > > Please don't post screenshots of text. It's pointlessly bloating the > > mail and I can't respond inline. Please re-post all the relevant bits as > > text. I've remembered a few other reasons to avoid screenshots. Mostly I can't copy&paste if I wanted to look for something you've posted and also the contents of the image are not indexed by search engines. Also please note that the reviews may seem nitpicky, but I'm trying to be thorough. By merging your code we'll need to support and maintain the code in the future even in cases when you decide to no longer participate in our project, thus we need to avoid as many future problems as possible. > Sorry about that, I didn't know what the policy on images was. Basically, > we used the REGENERATE_OUTPUT for the xml2xmltest and inside the generated > output there is a line which reads "<backingStore type='network' > index='1'>". However, the xml2xmltest fails on test 181 with the error: > Expect [ index='1'>] > Actual [>] > > This backingStore is the only place in the file with an index='1', so > logically we tried removing that attribute. However, this causes test 182 > to fail (even though it passed before this change) with the error: > Expect [>] > Actual [ index='1'>] > > It seems like these tests are at odds with one another and we aren't sure > what the cause or fix would be. If we use an index, then 181 fails, and if > we don't, then 182 will fail. Do you know what might be causing this? Ah, yes. That test is a bit magic when the definition XML of a live VM differs from a definition XML of an inactive VM and in such case VIR_TEST_REGENRATE_OUTPUT=1 will not do the right thing fully automatically. You need to create tests/qemuxml2xmloutdata/$TESTNAME-inactive.xml as an empty file and then re-REGENERATE the output. The presence of the -inactive file is registered by the test runner and then two distinct versions are allowed. Also note that if you provided full text output of the test I would be able to replace $TESTNAME by the actual file name you need to use, by copying and pasting. Also, in this case it wasn't necessary, but always provide at least link to your repository if you've modified the code. Making reviewers job as easy as possible is the best way to expedite the review process. Review is a rather scarse resource in many projects, so having the reviwer guess or look for additional data wastes the available resource.
