| Message ID | 70524bebab26f8a98b7acd69e9387676c5481b54.1452298382.git.crobinso@redhat.com |
|---|---|
| State | Accepted |
| Commit | 825d357a467d4ca2e80d6d49e0fdd33afee07605 |
| Headers | show |
On 01/08/2016 08:59 PM, Laine Stump wrote: > On 01/08/2016 07:13 PM, Cole Robinson wrote: >> Since test files are formatted predictably nowadays, we can make >> VIR_TEST_REGENERATE_OUTPUT handle most cases for us with a simple >> replacement. test-wrap-argv.pl is still canon, but this bit makes >> it easier to confirm test output changes during active development. >> --- >> tests/testutils.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/tests/testutils.c b/tests/testutils.c >> index 6645d61..0091fcd 100644 >> --- a/tests/testutils.c >> +++ b/tests/testutils.c >> @@ -469,10 +469,19 @@ virtTestDifferenceFullInternal(FILE *stream, >> actualStart = actual; >> actualEnd = actual + (strlen(actual)-1); >> - if (regenerate && virTestGetRegenerate() > 0) { >> + if (regenerate && (virTestGetRegenerate() > 0) && expectName && actual) { >> + char *regencontent; >> + >> + /* Try to properly indent qemu argv files */ >> + if (!(regencontent = virStringReplace(actual, " -", " \\\n-"))) >> + return -1; >> + >> if (expectName && actual && >> - virFileWriteStr(expectName, actual, 0666) < 0) >> + virFileWriteStr(expectName, regencontent, 0666) < 0) { >> + VIR_FREE(regencontent); >> return -1; >> + } >> + VIR_FREE(regencontent); >> } >> if (!virTestGetDebug()) > > ACK (although the whole REGENERATE_OUTPUT thing makes me worry that someone > may be lulled into blindly regenerating the test output in some case where the > test really is catching a regression) Yeah it's a risk, but that's what code review is for :) The alternative of hand editing XML files is exhausting I've pushed these patches now, thanks for the review! - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/12/2016 08:29 AM, John Ferlan wrote: > > > On 01/08/2016 09:21 PM, Cole Robinson wrote: >> On 01/08/2016 08:59 PM, Laine Stump wrote: >>> On 01/08/2016 07:13 PM, Cole Robinson wrote: >>>> Since test files are formatted predictably nowadays, we can make >>>> VIR_TEST_REGENERATE_OUTPUT handle most cases for us with a simple >>>> replacement. test-wrap-argv.pl is still canon, but this bit makes >>>> it easier to confirm test output changes during active development. >>>> --- >>>> tests/testutils.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> > > FWIW: Coverity has a complaint... > >>>> diff --git a/tests/testutils.c b/tests/testutils.c >>>> index 6645d61..0091fcd 100644 >>>> --- a/tests/testutils.c >>>> +++ b/tests/testutils.c >>>> @@ -469,10 +469,19 @@ virtTestDifferenceFullInternal(FILE *stream, >>>> actualStart = actual; >>>> actualEnd = actual + (strlen(actual)-1); >>>> - if (regenerate && virTestGetRegenerate() > 0) { >>>> + if (regenerate && (virTestGetRegenerate() > 0) && expectName && actual) { > > (3) Event check_after_deref: Null-checking "actual" suggests that it > may be null, but it has already been dereferenced on all paths leading > to the check. > > > Also of note is a few lines earlier: > > 464 if (!actual) > 465 actual = ""; > > > Did perhaps you want to see if actual was an empty string? > There's a few bits wrong here, I didn't look closely at the new code when rebasing this patch. I'll send a fix. Thanks, Cole > John >>>> + char *regencontent; >>>> + >>>> + /* Try to properly indent qemu argv files */ >>>> + if (!(regencontent = virStringReplace(actual, " -", " \\\n-"))) >>>> + return -1; >>>> + >>>> if (expectName && actual && >>>> - virFileWriteStr(expectName, actual, 0666) < 0) >>>> + virFileWriteStr(expectName, regencontent, 0666) < 0) { >>>> + VIR_FREE(regencontent); >>>> return -1; >>>> + } >>>> + VIR_FREE(regencontent); >>>> } >>>> if (!virTestGetDebug()) >>> >>> ACK (although the whole REGENERATE_OUTPUT thing makes me worry that someone >>> may be lulled into blindly regenerating the test output in some case where the >>> test really is catching a regression) >> >> Yeah it's a risk, but that's what code review is for :) The alternative of >> hand editing XML files is exhausting >> >> I've pushed these patches now, thanks for the review! >> >> - Cole >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/tests/testutils.c b/tests/testutils.c index 6645d61..0091fcd 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -469,10 +469,19 @@ virtTestDifferenceFullInternal(FILE *stream, actualStart = actual; actualEnd = actual + (strlen(actual)-1); - if (regenerate && virTestGetRegenerate() > 0) { + if (regenerate && (virTestGetRegenerate() > 0) && expectName && actual) { + char *regencontent; + + /* Try to properly indent qemu argv files */ + if (!(regencontent = virStringReplace(actual, " -", " \\\n-"))) + return -1; + if (expectName && actual && - virFileWriteStr(expectName, actual, 0666) < 0) + virFileWriteStr(expectName, regencontent, 0666) < 0) { + VIR_FREE(regencontent); return -1; + } + VIR_FREE(regencontent); } if (!virTestGetDebug())