980788 - [manifestparser] Add greater-than/less-than (equal) support

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ted Mielczarek [:ted.mielczarek] from bug 964278 comment 3)
Note that the expression parser we use in manifestparser doesn't currently support greater-than/less-than comparisons:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py#36

Comment 1

[Luv Agarwal(:lagarwal)]

Hey, i am interested in taking this up. Please assign this to me.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Luv! I have assigned the bug to you. Andrew, would you mind to mentor this bug? Or is there someone else?

Comment 3

[Luv Agarwal(:lagarwal)]

Henrick, at mxr i searched for the terms run-if, skip-if, fail-if tags but cannot find any of these tags using queries involving version numbers (as [:ahal] said in bug 964278 comment 4) or something which would require the use of ge, le, gt or lt, operators. can you please provide me with cases where you would use the above operators so that i can add them in re.Scanner (http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py#202) and write their token functions.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, we don't have those terms yet, because we cannot make use of it. Maybe some cases you can find in our mozmill-tests repository (https://hg.mozilla.org/qa/mozmill-tests), where we workaround this problem by adding skip code inside the test itself. This is something we would like to get rid of.

Comment 5

[Andrew Halberstadt [:ahal]]

:wlach recently pointed out to me that there's an undocumented builtin class that can compare version strings. It's distutils.version.StrictVersion, so e.g:

from distutils.version import StrictVersion
print StrictVersion('0.10') > StrictVersion('0.2')

The tricky part is knowing what type we are trying to compare. Manifestparser will either need to guess whether a token is a version string or not, or be explicitly told what constitutes a version string (e.g in the manifests we could say you need to put 'version()' around the version string).

Another question is whether this should be built-in to manifestparser, or whether individual harnesses can "plug in" types at runtime. I think I prefer the latter approach, though it is trickier to implement. For example the harness could do something like this:

class VersionString(object):
    def __init__(self, token):
        token = token[len('version('):-1]
        self.token = StrictVersion(token)

    @staticmethod
    def is_type(token):
        return token.startswith('version')

    def __eq__(self, other):
        return cmp(self.token, other)

    def __lt__(self, other):
        ...
    ...

manifest = TestManifest(..., custom_types=[VersionString])

Then when parsing, manifestparser would first check to see if a token belongs to a custom_type before proceeding as normal.

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Luv Agarwal(:lagarwal) from comment #3)
> Henrick, at mxr i searched for the terms run-if, skip-if, fail-if tags but
> cannot find any of these tags using queries involving version numbers (as
> [:ahal] said in bug 964278 comment 4) or something which would require the
> use of ge, le, gt or lt, operators. can you please provide me with cases
> where you would use the above operators so that i can add them in re.Scanner
> (http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/
> manifestparser/manifestparser/manifestparser.py#202) and write their token
> functions.

Just to clarify, this bug has nothing to do with version strings (other than it blocks bug 964278). In this bug we need a way to compare "arbitrary things" one of which happens to be version strings.

Comment 7

[Gregory Szorc [:gps]]

StrictVersion() will raise ValueError if the argument can't be parsed into a version. FWIW, there's also LooseVersion for all your non-standard X.Y.Z version strings.

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> :wlach recently pointed out to me that there's an undocumented builtin class
> that can compare version strings. It's distutils.version.StrictVersion, 

I discussed something like this in bug 964278. It should be fine to do this, since we can put arbitrary objects into the mozinfo dictionary, so we could do something like:

class MyVersion(StrictVersion):
  def __lt__(self, other):
    if isinstance(other, basestring):
      return self < StrictVersion(other)

mozinfo.info['version'] = MyVersion('1.0')

[test_foo]
skip-if = version < '2.0'

Comment 9

[Andrew Halberstadt [:ahal]]

Oh good call, but if we go that route then doing:

[test_foo]
skip-if = '1.0' < version

wouldn't work. Not a huge problem, but we'd probably want to make sure we error out if anyone does write a manifest like that.

Comment 10

[(not currently active) Ted Mielczarek]

FYI, Python is actually pretty cool about this:
===================================
class Foo:
    def __lt__(self, other):
        if isinstance(other, str):
            return 5

    def __gt__(self, other):
        if isinstance(other, str):
            return 6

f = Foo()
print "f < 'a': %s" % (f < 'a')
print "'a' < f: %s" % ('a' < f)
===================================
$ python /tmp/test.py 
f < 'a': 5
'a' < f: 6

Comment 11

[Luv Agarwal(:lagarwal)]

It seems that this bug is short of information, so i am not able to proceed.

Andrew, what are the arbitrary things other than version strings that i should take into account.

Comment 12

[Andrew Halberstadt [:ahal]]

It should be possible to compare any classes that define rich comparison methods like __lt__ or __gt__ like in ted's example above (see https://docs.python.org/2/reference/datamodel.html#object.__lt__ for more info).

In practical terms, all that means is don't do any type checking. Just compare two objects with '<' or '>' and let python determine the result.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

resetting the assigned to field as there has been no action on this bug.

Ahal, can you confirm that this is still a useful bug to fix?

Comment 14

[Andrew Halberstadt [:ahal]]

Yes, it is still valid.

Comment 15

[Julien Pagès (:parkouss)]

Attachment: 980788.patch

Added support for >, <, >=, <= to manifest parser expressions.

Tests run fine locally.

I did'nt asked for review yet, as I can't choose :ahal and I'm not sure who else I can ask for that.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8629239 [details] [diff] [review]
980788.patch

Review of attachment 8629239 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great! (get it?)

Just a few naming nits.

::: testing/mozbase/manifestparser/manifestparser/expression.py
@@ +81,5 @@
> +    "<="
> +    def led(self, parser, left):
> +        return left <= parser.expression(self.lbp)
> +
> +class get_op_token(object):

nit: I've never seen 'let' or 'get' before, usually it's 'le' and 'ge'. Please update the class names.

::: testing/mozbase/manifestparser/tests/test_expressionparser.py
@@ +90,5 @@
>          self.assertTrue(parse("true || !true)"))
>          self.assertFalse(parse("!true && true"))
>          self.assertFalse(parse("true && !true"))
>  
> +    def test_lower_than(self):

nit: the 'l' actually stands for 'less' or 'lesser'

@@ +118,5 @@
> +        self.assertTrue(parse("'def' > 'abc'"))
> +        self.assertFalse(parse("1 > 1"))
> +        self.assertFalse(parse("'abc' > 'abc'"))
> +
> +    def test_lower_or_equals_than(self):

ditto.

Comment 17

[Julien Pagès (:parkouss)]

Thanks Andrew!

I fixed the nits, and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31649211814c

Comment 18

[Julien Pagès (:parkouss)]

Attachment: 980788_fixed.patch

Try is green after the fixes, so carrying previsous r+ from :ahal and asking for checkin-needed.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8629239 [details] [diff] [review]
980788.patch

Review of attachment 8629239 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/manifestparser/manifestparser/expression.py
@@ +139,1 @@
>                (eq_op_token, neq_op_token),

I think you want these to be equal precedence with the existing eq / neq tokens, otherwise we could get surprising parse results.

Comment 20

[Julien Pagès (:parkouss)]

Right, thanks Ted. :)

Comment 21

[Julien Pagès (:parkouss)]

Attachment: 980788_re_fixed.patch

Fixed with :ted suggestion. I ran the tests locally with success, so I'm confident and another try push should not be needed here.

Carrying r+ from the original review by :ahal.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9dd993c734

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e9dd993c734