11:22 a.m.
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(a)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
2:19 p.m.
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(a)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(a)redhat.com>
John
unsigned int virTestGetDebug(void);
unsigned int virTestGetVerbose(void);
1:37 a.m.
On 2/19/19 9:19 PM, John Ferlan wrote:
Assuming extraction (sigh) of the VIR_AUTOFREE,
Reviewed-by: John Ferlan<jferlan(a)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
6:33 a.m.
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(a)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
6:56 a.m.
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
7:06 a.m.
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
7:32 a.m.
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
2135
days inactive
2136
days old
6 comments
3 participants
participants (3)
-
Andrea Bolognani -
John Ferlan -
Michal Privoznik