Re: [openstack-dev] [nova] Proposal new hacking rules
On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote: I think pointing out that the default failure message for testtools.TestCase.assertEqual() uses the terms reference (expected) and actual is a reason why reviewers *should* ask patch submitters to use (expected, actual) ordering. Is there any reason for this specific ordering? Not sure about others, but I tend to write equality comparisons like this if var == 1: instead of if 1 == var: (although I've seen the latter in C code before). This gives rise to assert var == 1 or, moving into `unittest` domain assertEqual(var, 1) reading it as 'Assert `var` equals 1', which makes me wonder why the `assertEqual` API is defined the other way around (unlike how I'd write any other equality check). Nicolas ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/26/2014 06:20 AM, Nicolas Trangez wrote: On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote: I think pointing out that the default failure message for testtools.TestCase.assertEqual() uses the terms reference (expected) and actual is a reason why reviewers *should* ask patch submitters to use (expected, actual) ordering. Is there any reason for this specific ordering? Not sure about others, but I tend to write equality comparisons like this if var == 1: instead of if 1 == var: (although I've seen the latter in C code before). This gives rise to assert var == 1 or, moving into `unittest` domain assertEqual(var, 1) reading it as 'Assert `var` equals 1', which makes me wonder why the `assertEqual` API is defined the other way around (unlike how I'd write any other equality check). It's not about an equality condition. It's about the message that is produced by testtools.TestCase.assertEqual(), and the helpfulness of that message when the order of the arguments is reversed. This is especially true with large dict comparisons. If you get a message like: reference: large_dict actual: large_dict And the arguments are reversed, then you end up wasting time looking in the test code instead of the real code for the thing that is different. Anyway, like I said, it's not something that we can write a simple hacking check for, and therefore, it's not something that should have much time spent on. But I do recommend that reviewers bring it up, especially if the patch author has been inconsistent in their usage of (expected, actual) in multiple assertEqual() calls in their patch. Best, -jay ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Wed, 2014-11-26 at 08:54 -0500, Jay Pipes wrote: On 11/26/2014 06:20 AM, Nicolas Trangez wrote: On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote: I think pointing out that the default failure message for testtools.TestCase.assertEqual() uses the terms reference (expected) and actual is a reason why reviewers *should* ask patch submitters to use (expected, actual) ordering. Is there any reason for this specific ordering? Not sure about others, but I tend to write equality comparisons like this if var == 1: instead of if 1 == var: (although I've seen the latter in C code before). This gives rise to assert var == 1 or, moving into `unittest` domain assertEqual(var, 1) reading it as 'Assert `var` equals 1', which makes me wonder why the `assertEqual` API is defined the other way around (unlike how I'd write any other equality check). It's not about an equality condition. It's about the message that is produced by testtools.TestCase.assertEqual(), and the helpfulness of that message when the order of the arguments is reversed. I'm aware of that. I was just wondering whether there's a particular reason the ordering (and as a result of that the error message) was chosen as it is. I'd rather design the API as `assertEqual(value, expected)`, and let the message indeed say 'Expected ..., but got ...' (and using the argument values accordingly). Nicolas ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Wed, Nov 26, 2014 at 08:54:35AM -0500, Jay Pipes wrote: It's not about an equality condition. It's about the message that is produced by testtools.TestCase.assertEqual(), and the helpfulness of that message when the order of the arguments is reversed. This is especially true with large dict comparisons. If you get a message like: reference: large_dict actual: large_dict And the arguments are reversed, then you end up wasting time looking in the test code instead of the real code for the thing that is different. Anyway, like I said, it's not something that we can write a simple hacking check for, and therefore, it's not something that should have much time spent on. But I do recommend that reviewers bring it up, especially if the patch author has been inconsistent in their usage of (expected, actual) in multiple assertEqual() calls in their patch. I think Nicolas's question was what made testtools choose this ordering. As far as I know, the python docs for unittest uses the opposite ordering. I think most people can see that the error messages involving 'reference' and 'actual' are useful, but maybe not the fact that in order to achieve them using testtools, you need to go against the norm for other testing frameworks. (fwiw, I'm an advocate for using the ordering with the best error messages) signature.asc Description: Digital signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/26/2014 09:28 AM, Nicolas Trangez wrote: On Wed, 2014-11-26 at 08:54 -0500, Jay Pipes wrote: On 11/26/2014 06:20 AM, Nicolas Trangez wrote: On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote: I think pointing out that the default failure message for testtools.TestCase.assertEqual() uses the terms reference (expected) and actual is a reason why reviewers *should* ask patch submitters to use (expected, actual) ordering. Is there any reason for this specific ordering? Not sure about others, but I tend to write equality comparisons like this if var == 1: instead of if 1 == var: (although I've seen the latter in C code before). This gives rise to assert var == 1 or, moving into `unittest` domain assertEqual(var, 1) reading it as 'Assert `var` equals 1', which makes me wonder why the `assertEqual` API is defined the other way around (unlike how I'd write any other equality check). It's not about an equality condition. It's about the message that is produced by testtools.TestCase.assertEqual(), and the helpfulness of that message when the order of the arguments is reversed. I'm aware of that. I was just wondering whether there's a particular reason the ordering (and as a result of that the error message) was chosen as it is. I'd rather design the API as `assertEqual(value, expected)`, and let the message indeed say 'Expected ..., but got ...' (and using the argument values accordingly). I think you'd have the same problem, no? People would still need to get the order of the arguments correct. -jay ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/26/2014 07:54 AM, Jay Pipes wrote: On 11/26/2014 06:20 AM, Nicolas Trangez wrote: On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote: I think pointing out that the default failure message for testtools.TestCase.assertEqual() uses the terms reference (expected) and actual is a reason why reviewers *should* ask patch submitters to use (expected, actual) ordering. Is there any reason for this specific ordering? Not sure about others, but I tend to write equality comparisons like this if var == 1: instead of if 1 == var: (although I've seen the latter in C code before). This gives rise to assert var == 1 or, moving into `unittest` domain assertEqual(var, 1) reading it as 'Assert `var` equals 1', which makes me wonder why the `assertEqual` API is defined the other way around (unlike how I'd write any other equality check). It's not about an equality condition. It's about the message that is produced by testtools.TestCase.assertEqual(), and the helpfulness of that message when the order of the arguments is reversed. This is especially true with large dict comparisons. If you get a message like: reference: large_dict actual: large_dict And the arguments are reversed, then you end up wasting time looking in the test code instead of the real code for the thing that is different. And my argument is that you're going to have to check the test code anyway, because without some sort of automated enforcement you can never be sure that the test author got it right. I don't personally see having to open a failing unit test as a huge burden anyway - I generally need to do that to see what the unit test is calling anyway. That said, I'm not personally bothered by this. I learned early on not to trust the expected, actual ordering so it makes no difference what the failure message is to me. I think we could save less experienced developers some aggravation by not claiming something that may not be true, but if people disagree I'm not inclined to spend a bunch of time bikeshedding about it either. :-) -Ben Anyway, like I said, it's not something that we can write a simple hacking check for, and therefore, it's not something that should have much time spent on. But I do recommend that reviewers bring it up, especially if the patch author has been inconsistent in their usage of (expected, actual) in multiple assertEqual() calls in their patch. Best, -jay ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 26/11/14 09:33, Louis Taylor wrote: On Wed, Nov 26, 2014 at 08:54:35AM -0500, Jay Pipes wrote: It's not about an equality condition. It's about the message that is produced by testtools.TestCase.assertEqual(), and the helpfulness of that message when the order of the arguments is reversed. This is especially true with large dict comparisons. If you get a message like: reference: large_dict actual: large_dict And the arguments are reversed, then you end up wasting time looking in the test code instead of the real code for the thing that is different. Anyway, like I said, it's not something that we can write a simple hacking check for, and therefore, it's not something that should have much time spent on. But I do recommend that reviewers bring it up, especially if the patch author has been inconsistent in their usage of (expected, actual) in multiple assertEqual() calls in their patch. I think Nicolas's question was what made testtools choose this ordering. As far as I know, the python docs for unittest uses the opposite ordering. I think most people can see that the error messages involving 'reference' and 'actual' are useful, but maybe not the fact that in order to achieve them using testtools, you need to go against the norm for other testing frameworks. The python docs for unittest mostly use 'first' and 'second' as the parameter names, and unittest doesn't distinguish between expected and actual in the default error messages. That said, some of the newer assertions like assertDictEqual do use expected and actual, _in that order_, the same as testtools does. The bottom line is that there are exactly two ways to do it, the entire world has now chosen one way and while I might otherwise have chosen differently for the same reasons as Nicolas, it would be absurd not to do it the same way. That said, the entire discussion is moot because it can't be checked automatically. cheers, Zane. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Fri, Nov 21, 2014 at 01:52:50PM -0500, Matthew Treinish wrote: On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote: Hey, I am not a Nova developer but I still have an opinion. Using boolean assertions I like what you propose. We should use and enforce the assert* that best matches the intention. It's about semantic and the more precise we are, the better. Using same order of arguments in equality assertions Why not. But I don't know how we can write a Hacking rule for this. So you may fix all the occurrences for this now, but it might get back in the future. Let write this rules in HACKING.rst, developers and reviewers are expected to read it. Ok I'll bite, besides the enforceability issue which you pointed out, it just doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B == A) and I honestly feel that it goes beyond nitpicking because of that. It's also a fallacy that there will always be an observed value and an expected value. For example: self.assertEqual(method_a(), method_b()) Which one is observed and which one is expected? I think this proposal is just reading into the parameter names a bit too much. Let developer to know about what values was expected when he broke a test during his development without looking into the testcase code. Operators can also want to know about what values was expected/observed without reading the code when executing tests. Using LOG.warn instead of LOG.warning I am -1 on this. The part that comes after LOG. (LOG.warning, LOG.error, LOG.debug, etc) is the log level, it's not a verb. In syslog, the well-known log level is warning so the correct method to use here is, imo, log.warning(). Well I have choiced 'warn' because there is less changes but I do not have any preference, just want something clear from Nova. Have you concidered submitting this hacking rules to the hacking project here : https://github.com/openstack-dev/hacking ? I am sure these new rules makes sense on other openstack projects. Make them accepted by Nova community first before to think about other projects ;) Jordan - Original Message - From: Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com To: OpenStack Development Mailing List (not for usage questions) openstack-dev@lists.openstack.org Sent: Friday, November 21, 2014 5:57:14 PM Subject: Re: [openstack-dev] [nova] Proposal new hacking rules On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote: On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: This is something we can call nitpiking or low priority. This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these. Yes as written this is low priority but something necessary for a project like Nova it is. Considered that I feel sad to take your time. Can I suggest you to take no notice of this and let's others developers working on Nova too do this job ? ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Fri, Nov 21, 2014 at 05:23:28PM -0500, Matthew Treinish wrote:
On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote:
On 11/21/2014 01:52 PM, Matthew Treinish wrote:
On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
Hey,
I am not a Nova developer but I still have an opinion.
Using boolean assertions
I like what you propose. We should use and enforce the assert* that best
matches the intention. It's about semantic and the more precise we are,
the better.
Using same order of arguments in equality assertions
Why not. But I don't know how we can write a Hacking rule for this. So
you may fix all the occurrences for this now, but it might get back in
the future.
Ok I'll bite, besides the enforceability issue which you pointed out, it
just
doesn't make any sense, you're asserting 2 things are equal: (A == B) ==
(B == A)
and I honestly feel that it goes beyond nitpicking because of that.
It's also a fallacy that there will always be an observed value and an
expected value. For example:
self.assertEqual(method_a(), method_b())
Which one is observed and which one is expected? I think this proposal is
just
reading into the parameter names a bit too much.
If you are using assertEqual with 2 variable values... you are doing
your test wrong.
I was originally in your camp. But honestly, the error message provided
to the user does say expected and observed, and teaching everyone that
you have to ignore the error message is a much harder thing to do than
flip the code to conform to it, creating less confusion.
Uhm, no it doesn't, the default error message is A != B. [1][2][3] (both
with
unittest and testtools) If the error message was like that, then sure saying
one way was right over the other would be fine, (assuming you didn't specify a
different error message) but, that's not what it does.
[1]
https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340
[2]
https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85
[3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508
...
File
/opt/stack/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py, line
348, in assertEqual
self.assertThat(observed, matcher, message)
File
/opt/stack/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py, line
433, in assertThat
raise mismatch_error
MismatchError: !=:
reference = {'nova_object.changes': ['cells'],
'nova_object.data': {...
actual= {'nova_object.changes': ['cells'],
'nova_object.data': {...
...
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Fri, Nov 21, 2014 at 10:30:59AM -0800, Joe Gordon wrote: On Fri, Nov 21, 2014 at 8:57 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote: On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: This is something we can call nitpiking or low priority. This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these. Yes as written this is low priority but something necessary for a project like Nova it is. Why do you think this is necessary? Your asking make sense; You/Nova is looking for engineering time to focus on other development more importants. I would to help with my humble experience. * Let developer a chance to know about what values was expected when he broke a test. * Let developer to know what to use between warn or warning instead of loosing time by looking in the module what was used or doing a coin flip. Contributors are expected to read HACKING.rst and some of these rules can be tested by gate. Considered that I feel sad to take your time. Can I suggest you to take no notice of this and let's others developers working on Nova too do this job ? As the maintainer of openstack-dev/hacking and as a nova core, I don't think this is worth doing at all. Nova already has enough on its plate and doesn't need extra code to review. My point was not to discredit your opinion (My phrasing can be wrong I'm non-native english) I believe that you and contributors in general like me are working to make Nova better. Usually in opensource software contributors are welcome to help even if it is to fix a typo and I was hoped to help. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.
2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.
3/ warn{ing} -
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
On the overarching point: There is no way to get started with
OpenStack, other than starting small. My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews. But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.
Linus' post on this from the LKML is almost a decade old (!) but worth reading.
https://lkml.org/lkml/2004/12/20/255
MG
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
Matthew Gilliard said on Mon, Nov 24, 2014 at 02:50:08PM +:
1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.
2/ expected/actual - I don't see any reason not to try to improve
in this area, even if it's difficult to automate.
3/ warn{ing} -
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
N331: Use LOG.warning due to compatibility with py3
Linus' post on this from the LKML is almost a decade old (!) but worth
reading.
https://lkml.org/lkml/2004/12/20/255
+1 on all points.
Alexis
--
Nova Engineer, HP Cloud. AKA lealexis, lxsli.
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/24/2014 08:50 AM, Matthew Gilliard wrote:
1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.
2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.
Personally I'd rather kill the expected, actual ordering and just have
first, second or something that doesn't imply which value is which.
Because it can't be automatically enforced, we'll _never_ fix all of the
expected, actual mistakes (and will continually introduce new ones), so
I'd prefer to eliminate the confusion by not requiring a specific ordering.
Alternatively I suppose we could require kwargs for expected and actual
in assertEqual. That would at least make it more obvious when someone
has gotten it backward, but again that's a ton of code churn for minimal
gain IMHO.
3/ warn{ing} -
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
On the overarching point: There is no way to get started with
OpenStack, other than starting small. My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews. But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.
Linus' post on this from the LKML is almost a decade old (!) but worth
reading.
https://lkml.org/lkml/2004/12/20/255
MG
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/24/2014 09:40 AM, Ben Nemec wrote:
On 11/24/2014 08:50 AM, Matthew Gilliard wrote:
1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.
2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.
Personally I'd rather kill the expected, actual ordering and just have
first, second or something that doesn't imply which value is which.
Because it can't be automatically enforced, we'll _never_ fix all of the
expected, actual mistakes (and will continually introduce new ones), so
I'd prefer to eliminate the confusion by not requiring a specific ordering.
++. It should be a part of review to ensure that the test (including
error messages) makes sense. Simply having a (seemingly costly to
implement and enforce) rule stating that something must adhere to a
pattern does not guarantee that.
assertEqual(expected, actual, msg=nom nom nom cookie cookie yum)
matches the pattern, but the message still doesn't necessarily provide
much worth.
Focusing on making tests informative and clear about what is thought to
be broken on failure seems to be the better target (imo).
Alternatively I suppose we could require kwargs for expected and actual
in assertEqual. That would at least make it more obvious when someone
has gotten it backward, but again that's a ton of code churn for minimal
gain IMHO.
3/ warn{ing} -
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
On the overarching point: There is no way to get started with
OpenStack, other than starting small. My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews. But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.
Linus' post on this from the LKML is almost a decade old (!) but worth reading.
https://lkml.org/lkml/2004/12/20/255
MG
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/24/2014 01:02 PM, pcrews wrote:
On 11/24/2014 09:40 AM, Ben Nemec wrote:
On 11/24/2014 08:50 AM, Matthew Gilliard wrote:
1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.
2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.
Personally I'd rather kill the expected, actual ordering and just have
first, second or something that doesn't imply which value is which.
Because it can't be automatically enforced, we'll _never_ fix all of the
expected, actual mistakes (and will continually introduce new ones), so
I'd prefer to eliminate the confusion by not requiring a specific
ordering.
++. It should be a part of review to ensure that the test (including
error messages) makes sense. Simply having a (seemingly costly to
implement and enforce) rule stating that something must adhere to a
pattern does not guarantee that.
So, as a proponent of the (expected, actual) parameter order thing, I'll
just say that I actually agree with Patrick and Ben about NOT having a
hacking rule for this. The reason is because of what Ben noted: there's
really no way to programmatically check for this.
assertEqual(expected, actual, msg=nom nom nom cookie cookie yum)
matches the pattern, but the message still doesn't necessarily provide
much worth.
Focusing on making tests informative and clear about what is thought to
be broken on failure seems to be the better target (imo).
Agreed. And for me, I think pointing out that the default failure
message for testtools.TestCase.assertEqual() uses the terms reference
(expected) and actual is a reason why reviewers *should* ask patch
submitters to use (expected, actual) ordering. I just don't think it's
something that can be hacking-rule-tested for...
Best,
-jay
Alternatively I suppose we could require kwargs for expected and actual
in assertEqual. That would at least make it more obvious when someone
has gotten it backward, but again that's a ton of code churn for minimal
gain IMHO.
3/ warn{ing} -
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
On the overarching point: There is no way to get started with
OpenStack, other than starting small. My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews. But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.
Linus' post on this from the LKML is almost a decade old (!) but
worth reading.
https://lkml.org/lkml/2004/12/20/255
MG
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
Whoops, that should say assertions not exceptions. - Original Message - From: Solly Ross sr...@redhat.com To: OpenStack Development Mailing List (not for usage questions) openstack-dev@lists.openstack.org Sent: Monday, November 24, 2014 12:00:44 AM Subject: Re: [openstack-dev] [nova] Proposal new hacking rules Well, at least the message for exceptions in Nova says expected and observed. I suspect that it's part of our custom test case classes. Best Regards, Solly Ross - Original Message - From: Matthew Treinish mtrein...@kortar.org To: OpenStack Development Mailing List (not for usage questions) openstack-dev@lists.openstack.org Sent: Friday, November 21, 2014 5:23:28 PM Subject: Re: [openstack-dev] [nova] Proposal new hacking rules On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote: On 11/21/2014 01:52 PM, Matthew Treinish wrote: On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote: Hey, I am not a Nova developer but I still have an opinion. Using boolean assertions I like what you propose. We should use and enforce the assert* that best matches the intention. It's about semantic and the more precise we are, the better. Using same order of arguments in equality assertions Why not. But I don't know how we can write a Hacking rule for this. So you may fix all the occurrences for this now, but it might get back in the future. Ok I'll bite, besides the enforceability issue which you pointed out, it just doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B == A) and I honestly feel that it goes beyond nitpicking because of that. It's also a fallacy that there will always be an observed value and an expected value. For example: self.assertEqual(method_a(), method_b()) Which one is observed and which one is expected? I think this proposal is just reading into the parameter names a bit too much. If you are using assertEqual with 2 variable values... you are doing your test wrong. I was originally in your camp. But honestly, the error message provided to the user does say expected and observed, and teaching everyone that you have to ignore the error message is a much harder thing to do than flip the code to conform to it, creating less confusion. Uhm, no it doesn't, the default error message is A != B. [1][2][3] (both with unittest and testtools) If the error message was like that, then sure saying one way was right over the other would be fine, (assuming you didn't specify a different error message) but, that's not what it does. [1] https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340 [2] https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85 [3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
Well, at least the message for exceptions in Nova says expected and observed. I suspect that it's part of our custom test case classes. Best Regards, Solly Ross - Original Message - From: Matthew Treinish mtrein...@kortar.org To: OpenStack Development Mailing List (not for usage questions) openstack-dev@lists.openstack.org Sent: Friday, November 21, 2014 5:23:28 PM Subject: Re: [openstack-dev] [nova] Proposal new hacking rules On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote: On 11/21/2014 01:52 PM, Matthew Treinish wrote: On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote: Hey, I am not a Nova developer but I still have an opinion. Using boolean assertions I like what you propose. We should use and enforce the assert* that best matches the intention. It's about semantic and the more precise we are, the better. Using same order of arguments in equality assertions Why not. But I don't know how we can write a Hacking rule for this. So you may fix all the occurrences for this now, but it might get back in the future. Ok I'll bite, besides the enforceability issue which you pointed out, it just doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B == A) and I honestly feel that it goes beyond nitpicking because of that. It's also a fallacy that there will always be an observed value and an expected value. For example: self.assertEqual(method_a(), method_b()) Which one is observed and which one is expected? I think this proposal is just reading into the parameter names a bit too much. If you are using assertEqual with 2 variable values... you are doing your test wrong. I was originally in your camp. But honestly, the error message provided to the user does say expected and observed, and teaching everyone that you have to ignore the error message is a much harder thing to do than flip the code to conform to it, creating less confusion. Uhm, no it doesn't, the default error message is A != B. [1][2][3] (both with unittest and testtools) If the error message was like that, then sure saying one way was right over the other would be fine, (assuming you didn't specify a different error message) but, that's not what it does. [1] https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340 [2] https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85 [3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote: On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: This is something we can call nitpiking or low priority. This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these. Yes as written this is low priority but something necessary for a project like Nova it is. Considered that I feel sad to take your time. Can I suggest you to take no notice of this and let's others developers working on Nova too do this job ? ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Fri, Nov 21, 2014 at 8:57 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote: On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: This is something we can call nitpiking or low priority. This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these. Yes as written this is low priority but something necessary for a project like Nova it is. Why do you think this is necessary? Considered that I feel sad to take your time. Can I suggest you to take no notice of this and let's others developers working on Nova too do this job ? As the maintainer of openstack-dev/hacking and as a nova core, I don't think this is worth doing at all. Nova already has enough on its plate and doesn't need extra code to review. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Fri, Nov 21, 2014 at 10:30:59AM -0800, Joe Gordon wrote: On Fri, Nov 21, 2014 at 8:57 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote: On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: This is something we can call nitpiking or low priority. This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these. Yes as written this is low priority but something necessary for a project like Nova it is. Why do you think this is necessary? Considered that I feel sad to take your time. Can I suggest you to take no notice of this and let's others developers working on Nova too do this job ? As the maintainer of openstack-dev/hacking and as a nova core, I don't think this is worth doing at all. Nova already has enough on its plate and doesn't need extra code to review. I tend to agree, also hacking rules are there to reduce the mental load of reviewers so that they have less to worry about doing a review. Honestly, none of these proposals seem like anything a reviewer should ever worry about in the first place, let alone a reason to -1 a patch. (either automatically with a rule or manually) pgp89v3VuxZ0N.pgp Description: PGP signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On 11/21/2014 01:52 PM, Matthew Treinish wrote: On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote: Hey, I am not a Nova developer but I still have an opinion. Using boolean assertions I like what you propose. We should use and enforce the assert* that best matches the intention. It's about semantic and the more precise we are, the better. Using same order of arguments in equality assertions Why not. But I don't know how we can write a Hacking rule for this. So you may fix all the occurrences for this now, but it might get back in the future. Ok I'll bite, besides the enforceability issue which you pointed out, it just doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B == A) and I honestly feel that it goes beyond nitpicking because of that. It's also a fallacy that there will always be an observed value and an expected value. For example: self.assertEqual(method_a(), method_b()) Which one is observed and which one is expected? I think this proposal is just reading into the parameter names a bit too much. If you are using assertEqual with 2 variable values... you are doing your test wrong. I was originally in your camp. But honestly, the error message provided to the user does say expected and observed, and teaching everyone that you have to ignore the error message is a much harder thing to do than flip the code to conform to it, creating less confusion. -Sean -- Sean Dague http://dague.net signature.asc Description: OpenPGP digital signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote: On 11/21/2014 01:52 PM, Matthew Treinish wrote: On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote: Hey, I am not a Nova developer but I still have an opinion. Using boolean assertions I like what you propose. We should use and enforce the assert* that best matches the intention. It's about semantic and the more precise we are, the better. Using same order of arguments in equality assertions Why not. But I don't know how we can write a Hacking rule for this. So you may fix all the occurrences for this now, but it might get back in the future. Ok I'll bite, besides the enforceability issue which you pointed out, it just doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B == A) and I honestly feel that it goes beyond nitpicking because of that. It's also a fallacy that there will always be an observed value and an expected value. For example: self.assertEqual(method_a(), method_b()) Which one is observed and which one is expected? I think this proposal is just reading into the parameter names a bit too much. If you are using assertEqual with 2 variable values... you are doing your test wrong. I was originally in your camp. But honestly, the error message provided to the user does say expected and observed, and teaching everyone that you have to ignore the error message is a much harder thing to do than flip the code to conform to it, creating less confusion. Uhm, no it doesn't, the default error message is A != B. [1][2][3] (both with unittest and testtools) If the error message was like that, then sure saying one way was right over the other would be fine, (assuming you didn't specify a different error message) but, that's not what it does. [1] https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340 [2] https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85 [3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508 pgp9OcDwSQb6e.pgp Description: PGP signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Proposal new hacking rules
On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui sahid.ferdja...@redhat.com wrote: This is something we can call nitpiking or low priority. This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these. I would like we introduce 3 new hacking rules to enforce the cohesion and consistency in the base code. Using boolean assertions Some tests are written with equality assertions to validate boolean conditions which is something not clean: assertFalse([]) asserts an empty list assertEqual(False, []) asserts an empty list is equal to the boolean value False which is something not correct. Some changes has been started here but still needs to be appreciated by community: * https://review.openstack.org/#/c/133441/ * https://review.openstack.org/#/c/119366/ Using same order of arguments in equality assertions Most of the code is written with assertEqual(Expected, Observed) but some part are still using the opposite. Even if they provide any real optimisation using the same convention help reviewing and keep a better consistency in the code. assertEqual(Expected, Observed) OK assertEqual(Observed, Expected) KO A change has been started here but still needs to be appreciated by community: * https://review.openstack.org/#/c/119366/ Using LOG.warn instead of LOG.warning - We can see many time reviewers -1ed a patch to ask developer to use 'warn' instead of 'warning'. This will provide no optimisation but let's finally have something clear about what we have to use. LOG.warning: 74 LOG.warn:319 We probably want to use 'warn' Nothing has been started from what I know. Thanks, s. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
