771030 - [manifestpaser] Add relative path to tests

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

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

For bug 771029 we need the relative path from the root manifest to the test itself. Therefore we have to determine it as given by the individual root manifest files.

Comment 1

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

Attachment: Patch

Pointer to Github pull-request

Comment 2

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

Jeff, do you have any update for the review request? The patch is waiting here already a week now. Thanks.

Comment 3

[Jeff Hammel]

Sorry, I have been trying to figure what to do about this line:

if self.read.__name__ is not sys._getframe(1).f_code.co_name:

What we do currently is, at best, questionable.  However, I don't think looking at the stack here is a good thing to do.  I can r- if you want resolution, but I don't precisely know what to do at this point.  I've also been trying to figure out where this is used downstream and what this will affect.

Comment 4

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

I was looking around on the web for that question and this method has been used all the time. So not sure if we have another chance as changing the API for that method, which I really didn't wanted to do. Why do you think looking at the stack is not a good thing? I kinda would like to get more information as only that sentence.

Comment 5

[Jeff Hammel]

Comment on attachment 639205 [details]
Patch

+ if self.read.__name__ is not sys._getframe(1).f_code.co_name:

> I was looking around on the web for that question and this method has been used all the time. So not sure if we have another chance as changing the API for that method, which I really didn't wanted to do. Why do you think looking at the stack is not a good thing? I kinda would like to get more information as only that sentence.

So there are a few things here.  Probably the most important is that it changes the API.  I'm not sure if we want to do this or not.  The way it is currently, the rootdir is the directory of the first manifest given to read() for a particular instance  The way you would change it to, self.rootdir is the directory of the first manifest of the last that read() was called.  I am not sure if we want this change.  

In addition, manifestparser is used by xpcshell.  I am not sure if in practice changing the way self.rootdir is determined would break xpcshell usage. http://mxr.mozilla.org/mozilla-central/source/build/xpccheck.py is one example.
It is also used by marionette. So I hesitate to change the API unless we really want the change and all consumers of the change are either agnostic to it or want it.

If we were to change the api such that the root directory is defined per read, I would probably want to get rid of self.rootdir entirely.  The way that it is now is certainly not ideal.  Another alternative is to get rid of it unless it is explicitly set.  If we want a root per top-level manifest, this isn't the way to do it, since we're setting an attribute on the class and not per manifest and not cleaning it up.  Instead, it is better to pass around the root that we want.

As far as the particular line of code, self.read.__name__ should be 'read' so I don't think there's a reason to abstract that.  In general, I think looking at the call stack as a method of last resort.  There are plenty of other ways of detect if you're at the top-level of a recursive function.  For one, the callstack can be easily fooled:

import sys
class foo(object):

    def read(self):
        print 'name: %s; co_name: %s' % (self.read.__name__, sys._getframe(1).f\
_code.co_name)
        if self.read.__name__ is not sys._getframe(1).f_code.co_name:
            self.read()

def read():
    foo().read()

print "The pledge:"
foo().read()

print "The turn:"
read()

# This however works

print "The prestige:"
class bar(object):
    def _read(self, root, args, kwargs):
        print 'name: %s; co_name: %s' % (self.read.__name__, sys._getframe(1).f\
_code.co_name)
        if root is not None:
            self._read(None, args, kwargs)

    def read(self, *args, **kwargs):
        root = 'here'
        self._read(root, args, kwargs)
bar().read()

The last case is in my opinion more explicit and a better design.  You could also put _read as an inner function of read which is probably better if one is really concerned with not allowing _read to be called other than by read.

If we want to change the API, I am personally fine with that though we should carefully consider what we're changing it to and we should probably make sure everyone is in the know about the change and that it doesn't break consumers. This is also fixable without changing the API.

Comment 6

[Jeff Hammel]

>The way you would change it to, self.rootdir is the directory of the first manifest of the last that read() was called. 

Sorry, I meant the *last* manifest of the last time that read() was called

Comment 7

[Jeff Hammel]

It is also worth noting that help(sys._getframe) ends with: "This function should be used for internal and specialized purposes only."

Comment 8

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

(In reply to Jeff Hammel [:jhammel] from comment #5)
> print "The prestige:"
> class bar(object):
>     def _read(self, root, args, kwargs):
>         print 'name: %s; co_name: %s' % (self.read.__name__,
> sys._getframe(1).f\
> _code.co_name)
>         if root is not None:
>             self._read(None, args, kwargs)
> 
>     def read(self, *args, **kwargs):
>         root = 'here'
>         self._read(root, args, kwargs)
> bar().read()

Well, that would mean a lot of duplicated code. I can't simple set root via here, because we need the filenames of each manifest. It's not a single root we will get but the rootPath has to be dependent on the current top-level manifest.

Personally I simply don't like passing in '*filenames, **defaults', because it makes it impossible to extend methods. Why hasn't been used a named list and dict instead?

Comment 9

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Jeff Hammel [:jhammel] from comment #5)
> > print "The prestige:"
> > class bar(object):
> >     def _read(self, root, args, kwargs):
> >         print 'name: %s; co_name: %s' % (self.read.__name__,
> > sys._getframe(1).f\
> > _code.co_name)
> >         if root is not None:
> >             self._read(None, args, kwargs)
> > 
> >     def read(self, *args, **kwargs):
> >         root = 'here'
> >         self._read(root, args, kwargs)
> > bar().read()
> 
> Well, that would mean a lot of duplicated code. I can't simple set root via
> here, because we need the filenames of each manifest. It's not a single root
> we will get but the rootPath has to be dependent on the current top-level
> manifest.

It shouldn't be any duplicate code.  You have a method, _read(), that is the internal method. Then you have a method, read(), that loops over the top-level manifests and calls _read which contains all of the logic.

> Personally I simply don't like passing in '*filenames, **defaults', because
> it makes it impossible to extend methods. Why hasn't been used a named list
> and dict instead?

The method signature was chosen as it fit with how a user would actually call it.  Generally you will have one or more manifests and may have default values to assign them.  If there was a reason to have any additional user-facing parameters, then the signature would have to be reconsidered.  So far I have not encountered such a reason

Comment 10

[Jeff Hammel]

Attachment: i believe this does what you want

and it doesn't break tests.  Feel free to use this as a basis to work on

https://github.com/k0s/mozbase/tree/bug-771030

Comment 11

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

So that means you are ok in only checking for the existence of the root manifest files and not for included sub-manifests? With that change we will no longer report that the referenced manifest doesn't exist but will bail out with a pure os level exception when trying to read the manifest.

Comment 12

[Jeff Hammel]

This is checked for and in the same manner as the current code, and it is checked for before recursing:

https://github.com/k0s/mozbase/blob/bug-771030/manifestdestiny/manifestparser/manifestparser.py#L424

Also, this patch is intended as a proof of concept and suggestion. I'm also not entirely sure whether it is better to whether to stick in the relative path to the top-level manifest's directory, or whether it is better to stick in (absolute) path to the top-level manifest.  The former may be computed from the latter and the former is ambiguous as to which manifest a particular test (ultimately) comes from.

(We currently also in both cases raise IOErrors vs OSErrors, which is probably wrong, but I did not change that.)

Comment 13

[Jeff Hammel]

Attachment: unbitrot

:whimboo, :wlach : do we want to take this fix?  If not we should probably close or figure out what to do with this bug.  Personally I'm not sure we need this functionality but it doesn't hurt anything to have

Comment 14

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

I would prefer if we could take it. It would help a lot for frameworks like mozmill when the relative path needs to be output given to the root manifest location. It's easier to add this value when collecting the tests as later when test results are checked.

Jeff, have you had a chance to check other frameworks if this causes failures? If nothing shows off I'm fine in going this route.

Comment 15

[Jeff Hammel]

Since this doesn't change the API, just adds the relpath to the dict for the test, it should be fine for existing frameworks

Comment 16

[William Lachance (:wlach)]

Comment on attachment 649320 [details] [diff] [review]
unbitrot

I'm not 100% sure I'm the best person to ask about this, but I think your solution looks pretty good to me.

--- a/manifestdestiny/manifestparser/manifestparser.py
+++ b/manifestdestiny/manifestparser/manifestparser.py
@@ -400,6 +400,56 @@ class ManifestParser(object):
     def getRelativeRoot(self, root):
         return root
 
+    def _read(self, root, filename, defaults):
+
+        # get directory of this file
+        here = os.path.dirname(os.path.abspath(filename))
+        defaults['here'] = here
+
+

^^^
I'm guessing you didn't mind to have two newlines here.

Comment 17

[Jeff Hammel]

(In reply to William Lachance (:wlach) from comment #16)
> Comment on attachment 649320 [details] [diff] [review]
> unbitrot
> 
> I'm not 100% sure I'm the best person to ask about this, but I think your
> solution looks pretty good to me.

I think only ted and me have worked on manifestparser much, so you're as good as any ;)

> --- a/manifestdestiny/manifestparser/manifestparser.py
> +++ b/manifestdestiny/manifestparser/manifestparser.py
> @@ -400,6 +400,56 @@ class ManifestParser(object):
>      def getRelativeRoot(self, root):
>          return root
>  
> +    def _read(self, root, filename, defaults):
> +
> +        # get directory of this file
> +        here = os.path.dirname(os.path.abspath(filename))
> +        defaults['here'] = here
> +
> +
> 
> ^^^
> I'm guessing you didn't mind to have two newlines here.

I'll change it :) But no, its a low priority to me.

I also realized that we don't want manifestparser to output the relpath for new manifests so I will change this too.

Comment 18

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/3680f8e01ec1a5ed83232c2aeb6e92fc953a4cc9

I think its a good idea to bump this and release.  Thoughts, :wlach, :whimboo?

Comment 19

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

(In reply to Jeff Hammel [:jhammel] from comment #18)
> I think its a good idea to bump this and release.  Thoughts, :wlach,
> :whimboo?

Sounds good. I will do it in a bit.