[libvirt] [PATCH] testutils: Explicitly name virTestCompare*() arguments
Currently, some arguments are called strcontent and strsrc, or content and src or some other combination. This makes it impossible to see at the first glance what argument is supposed to represent 'expected' value and which one represents 'actual' value. Rename the arguments to make it obvious. At the same time, rework virTestCompareToULL a bit so that local variables are named in the same fashion. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 51 +++++++++++++++++------------------------------ tests/testutils.h | 10 +++++----- 2 files changed, 23 insertions(+), 38 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index d2219ad21e..59c1d1fd6e 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -767,19 +767,19 @@ int virTestDifferenceBin(FILE *stream, } /* - * @param strcontent: String input content - * @param filename: File to compare strcontent against + * @param actual: String input content + * @param filename: File to compare @actual against * - * If @strcontent is NULL, it's treated as an empty string. + * If @actual is NULL, it's treated as an empty string. */ int -virTestCompareToFile(const char *strcontent, +virTestCompareToFile(const char *actual, const char *filename) { int ret = -1; char *filecontent = NULL; char *fixedcontent = NULL; - const char *cmpcontent = strcontent; + const char *cmpcontent = actual; if (!cmpcontent) cmpcontent = ""; @@ -814,43 +814,28 @@ virTestCompareToFile(const char *strcontent, return ret; } -/* - * @param content: Input content - * @param src: Source to compare @content against - */ int -virTestCompareToULL(unsigned long long content, - unsigned long long src) +virTestCompareToULL(unsigned long long expected, + unsigned long long actual) { - char *strcontent = NULL; - char *strsrc = NULL; - int ret = -1; + VIR_AUTOFREE(char *) expectedStr = NULL; + VIR_AUTOFREE(char *) actualStr = NULL; - if (virAsprintf(&strcontent, "%llu", content) < 0) - goto cleanup; + if (virAsprintf(&expectedStr, "%llu", expected) < 0) + return -1; - if (virAsprintf(&strsrc, "%llu", src) < 0) - goto cleanup; + if (virAsprintf(&actualStr, "%llu", actual) < 0) + return -1; - ret = virTestCompareToString(strcontent, strsrc); - - cleanup: - VIR_FREE(strcontent); - VIR_FREE(strsrc); - - return ret; + return virTestCompareToString(expectedStr, actualStr); } -/* - * @param strcontent: String input content - * @param strsrc: String source to compare strcontent against - */ int -virTestCompareToString(const char *strcontent, - const char *strsrc) +virTestCompareToString(const char *expected, + const char *actual) { - if (STRNEQ_NULLABLE(strcontent, strsrc)) { - virTestDifference(stderr, strcontent, strsrc); + if (STRNEQ_NULLABLE(expected, actual)) { + virTestDifference(stderr, expected, actual); return -1; } diff --git a/tests/testutils.h b/tests/testutils.h index 658f9053ad..1ed9f0b6d3 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -76,12 +76,12 @@ int virTestDifferenceBin(FILE *stream, const char *expect, const char *actual, size_t length); -int virTestCompareToFile(const char *strcontent, +int virTestCompareToFile(const char *actual, const char *filename); -int virTestCompareToString(const char *strcontent, - const char *strsrc); -int virTestCompareToULL(unsigned long long content, - unsigned long long src); +int virTestCompareToString(const char *expected, + const char *actual); +int virTestCompareToULL(unsigned long long expected, + unsigned long long actual); unsigned int virTestGetDebug(void); unsigned int virTestGetVerbose(void); -- 2.19.2
On 2/19/19 12:22 PM, Michal Privoznik wrote:
Currently, some arguments are called strcontent and strsrc, or content and src or some other combination. This makes it impossible to see at the first glance what argument is supposed to represent 'expected' value and which one represents 'actual' value. Rename the arguments to make it obvious.
At the same time, rework virTestCompareToULL a bit so that local variables are named in the same fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 51 +++++++++++++++++------------------------------ tests/testutils.h | 10 +++++----- 2 files changed, 23 insertions(+), 38 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index d2219ad21e..59c1d1fd6e 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -767,19 +767,19 @@ int virTestDifferenceBin(FILE *stream, }
/* - * @param strcontent: String input content - * @param filename: File to compare strcontent against + * @param actual: String input content + * @param filename: File to compare @actual against * - * If @strcontent is NULL, it's treated as an empty string. + * If @actual is NULL, it's treated as an empty string. */ int -virTestCompareToFile(const char *strcontent, +virTestCompareToFile(const char *actual, const char *filename) { int ret = -1; char *filecontent = NULL; char *fixedcontent = NULL; - const char *cmpcontent = strcontent; + const char *cmpcontent = actual;
if (!cmpcontent) cmpcontent = ""; @@ -814,43 +814,28 @@ virTestCompareToFile(const char *strcontent, return ret; }
-/* - * @param content: Input content - * @param src: Source to compare @content against - */ int -virTestCompareToULL(unsigned long long content, - unsigned long long src) +virTestCompareToULL(unsigned long long expected,
Consistency with other virTestDifference* APIs would use "expect" rather than "expected". IDC which way you go though.
+ unsigned long long actual) { - char *strcontent = NULL; - char *strsrc = NULL; - int ret = -1; + VIR_AUTOFREE(char *) expectedStr = NULL; + VIR_AUTOFREE(char *) actualStr = NULL;
This conversion to VIR_AUTOFREE probably should be separate...
- if (virAsprintf(&strcontent, "%llu", content) < 0) - goto cleanup; + if (virAsprintf(&expectedStr, "%llu", expected) < 0) + return -1;
- if (virAsprintf(&strsrc, "%llu", src) < 0) - goto cleanup; + if (virAsprintf(&actualStr, "%llu", actual) < 0) + return -1;
- ret = virTestCompareToString(strcontent, strsrc); - - cleanup: - VIR_FREE(strcontent); - VIR_FREE(strsrc); - - return ret; + return virTestCompareToString(expectedStr, actualStr); }
-/* - * @param strcontent: String input content - * @param strsrc: String source to compare strcontent against - */ int -virTestCompareToString(const char *strcontent, - const char *strsrc) +virTestCompareToString(const char *expected,
similar "expect"
+ const char *actual) { - if (STRNEQ_NULLABLE(strcontent, strsrc)) { - virTestDifference(stderr, strcontent, strsrc); + if (STRNEQ_NULLABLE(expected, actual)) { + virTestDifference(stderr, expected, actual); return -1; }
diff --git a/tests/testutils.h b/tests/testutils.h index 658f9053ad..1ed9f0b6d3 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -76,12 +76,12 @@ int virTestDifferenceBin(FILE *stream, const char *expect, const char *actual, size_t length); -int virTestCompareToFile(const char *strcontent, +int virTestCompareToFile(const char *actual, const char *filename);
Is it just me that's bothered by just 1 of these not being like the others with expect as param1 and actual as param2. Different problem though.
-int virTestCompareToString(const char *strcontent, - const char *strsrc); -int virTestCompareToULL(unsigned long long content, - unsigned long long src); +int virTestCompareToString(const char *expected, + const char *actual); +int virTestCompareToULL(unsigned long long expected, + unsigned long long actual);
If you change the expected to expect don't forget these. Assuming extraction (sigh) of the VIR_AUTOFREE, Reviewed-by: John Ferlan <jferlan@redhat.com> John
unsigned int virTestGetDebug(void); unsigned int virTestGetVerbose(void);
On 2/19/19 9:19 PM, John Ferlan wrote:
Assuming extraction (sigh) of the VIR_AUTOFREE,
Reviewed-by: John Ferlan<jferlan@redhat.com>
While I'd definitely want this to be split into two patches if it was fixing something under src/, but this is under tests/ and therefore I did not bother. The reason for splitting a patch into smaller semanticaly divided patches is to help distro maintainers to ease backports. I don't think they will need to backport this patch, nor will they want only a part of it. But I can do the split if you still want me to. I'm postponing the push for now. Thanks, Michal
On 2/20/19 2:37 AM, Michal Privoznik wrote:
On 2/19/19 9:19 PM, John Ferlan wrote:
Assuming extraction (sigh) of the VIR_AUTOFREE,
Reviewed-by: John Ferlan<jferlan@redhat.com>
While I'd definitely want this to be split into two patches if it was fixing something under src/, but this is under tests/ and therefore I did not bother. The reason for splitting a patch into smaller semanticaly divided patches is to help distro maintainers to ease backports. I don't think they will need to backport this patch, nor will they want only a part of it.
Just going with precedent I've seen for other patches regardless of where they're found in the tree. Personally, I'm fine with doing it all at once. But I think perhaps some one should take the time to write down what the "house rules" are on the hacking page. Makes it easier that way. Go ahead with one patch - John
But I can do the split if you still want me to.
I'm postponing the push for now.
Thanks, Michal
On Wed, 2019-02-20 at 07:33 -0500, John Ferlan wrote:
On 2/20/19 2:37 AM, Michal Privoznik wrote:
On 2/19/19 9:19 PM, John Ferlan wrote:
Assuming extraction (sigh) of the VIR_AUTOFREE,
While I'd definitely want this to be split into two patches if it was fixing something under src/, but this is under tests/ and therefore I did not bother. The reason for splitting a patch into smaller semanticaly divided patches is to help distro maintainers to ease backports.
And reviewers, and people digging through history possibly years down the line. Whether the change is in src/, tests/ or whatever else shouldn't make a difference, it's all code and we routinely have to fix bugs both in the library and in the corresponding test suite. There is barely ever a reason *not* to split changes into smaller, independent units; having to write "at the same time" in the commit message should be your hint that you're doing it wrong ;)
I don't think they will need to backport this patch, nor will they want only a part of it.
Just going with precedent I've seen for other patches regardless of where they're found in the tree. Personally, I'm fine with doing it all at once. But I think perhaps some one should take the time to write down what the "house rules" are on the hacking page. Makes it easier that way.
We *kinda* have that already: Split large changes into a series of smaller patches, self-contained if possible [...] Perhaps we could reword that passage so it's clearer, and extend it by going into more detail. -- Andrea Bolognani / Red Hat / Virtualization
On 2/20/19 1:56 PM, Andrea Bolognani wrote:
On Wed, 2019-02-20 at 07:33 -0500, John Ferlan wrote:
On 2/20/19 2:37 AM, Michal Privoznik wrote:
On 2/19/19 9:19 PM, John Ferlan wrote:
Assuming extraction (sigh) of the VIR_AUTOFREE,
While I'd definitely want this to be split into two patches if it was fixing something under src/, but this is under tests/ and therefore I did not bother. The reason for splitting a patch into smaller semanticaly divided patches is to help distro maintainers to ease backports.
And reviewers, and people digging through history possibly years down the line.
Whether the change is in src/, tests/ or whatever else shouldn't make a difference, it's all code and we routinely have to fix bugs both in the library and in the corresponding test suite.
There is barely ever a reason *not* to split changes into smaller, independent units; having to write "at the same time" in the commit message should be your hint that you're doing it wrong ;)
Let me see, the whole sentence reads as follows: At the same time, rework virTestCompareToULL a bit so that local variables are named in the same fashion. I don't see any problem with that. But okay, I'll send v2 splitting this tiny patch into even smaller ones :-P Michal
On Wed, 2019-02-20 at 14:06 +0100, Michal Privoznik wrote:
On 2/20/19 1:56 PM, Andrea Bolognani wrote:
There is barely ever a reason *not* to split changes into smaller, independent units; having to write "at the same time" in the commit message should be your hint that you're doing it wrong ;)
Let me see, the whole sentence reads as follows:
At the same time, rework virTestCompareToULL a bit so that local variables are named in the same fashion.
I don't see any problem with that.
Okay, full disclosure: I clearly didn't pay enough attention while reading the commit message and thought the "at the same time" bit referred to the VIR_AUTOFREE() conversion. My bad. The overall point still stands, though.
But okay, I'll send v2 splitting this tiny patch into even smaller ones :-P
I'll check those out right away :) -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
John Ferlan -
Michal Privoznik