Closed Bug 1023790 Opened 10 years ago Closed 10 years ago

Enhance manifestparser for manifest files to provide a "parent" link

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: andrei, Assigned: andrei)

References

Details

(Whiteboard: [sprint])

Attachments

(1 file, 10 obsolete files)

18.14 KB, patch
andrei
: review+
Details | Diff | Splinter Review
This should work as a reverse to the [include] method.

This way we will be able to add default options in a root manifest file, and even if we only want to run a subset of tests, we will still have the projects defaults.

In particular we want this for specifying the docroot for mozhttpd in mozmill-tests (see bug  754847). We will be able to only specify it once in the root manifest file and even if we run a subset of the tests we'll still have access to this value.

I'm not sure yet of the implementation. We don't need to create the whole manifest tree from the bottom up, only follow the path to the root manifest file (the one that doesn't include the [parent] option) and apply the [DEFAULT] options.
Attached patch WIP 1 (obsolete) — Splinter Review
I've toyed with this a little today.
Marking this as WIP.

What we've done here is to add another section type, very similar to [include:]
I've named it [parent:]. And with it you set the manifest relationship from child to its parent.
> [parent:../parent.ini]

For parent manifest files we don't want to read the actual tests, we only want set variables. I've added a new argument to the _read method to ignore tests (probably should name it differently). And when this is True we return the read defaults.
Which are stored on a new object property named _parent_defaults with which we extends the data object.
Attachment #8438476 - Flags: feedback?(hskupin)
Comment on attachment 8438476 [details] [diff] [review]
WIP 1

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

I think its better to get feedback from William in that early state, and if he agrees on that helpful feature.
Attachment #8438476 - Flags: feedback?(hskupin) → feedback?(wlachance)
Comment on attachment 8438476 [details] [diff] [review]
WIP 1

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

This seems fine to me, tbh I don't have much opinion on manifests. Someone like jmaher might be good to ask, he's done much more work with manifests than I have.
Attachment #8438476 - Flags: feedback?(wlachance)
Attachment #8438476 - Flags: feedback?(jmaher)
Attachment #8438476 - Flags: feedback+
Comment on attachment 8438476 [details] [diff] [review]
WIP 1

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

I assume there is no easier method for including the base manifest first?  This general concept seems like a problem we could solve by fixing the tests and not solving a corner case.

Otherwise a few questions, this doesn't look too horrible and it has unit tests!

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +406,5 @@
>      """read .ini manifests"""
>  
>      def __init__(self, manifests=(), defaults=None, strict=True):
>          self._defaults = defaults or {}
> +        self._parent_defaults = {}

why is this at the root of the class level?

@@ +482,5 @@
> +                        raise IOError(message)
> +                    else:
> +                        sys.stderr.write("%s\n" % message)
> +                        continue
> +                self._parent_defaults = self._read(root, parent_file, {}, True)

why not do a data.update(self._read(root, parent_file, {}, True))

@@ +489,2 @@
>              # otherwise an item
> +            data.update(self._parent_defaults)

why not check for ignore_tests here and return, save the work.
how does this work if the parent manifest has a parent manifest, what does self._parent include?
Attachment #8438476 - Flags: feedback?(jmaher) → feedback+
Attached patch WIP 2 (obsolete) — Splinter Review
Better version.
Works up the tree. Defaults _should_ be saved inverse as the traversal.

But the more I have worked on this, the more I think this is a bad idea.
Basically whenever we are down-traversing a manifest tree structure, at every level, if there's a parent include we would also up-traverse the tree. I've made sure to exit sooner, but we would still parse the whole tree for _every_ parent include we come across.

Also the DEFAULT values we want to make sure will overwrite themselves properly.
In a parent > child > grandchild structure `grandchild` should overwrite the values set by all ancestors. While we are up-traversing the tree. This seems to get overcomplex quickly. State (default values) should be kept with a base-level priority, yet if we have includes we also need to traverse down where we would need to reset the base, re-traverse up the tree etc.

While in basic terms the current patch works (or at least should work). I'm inclined to drop this issue and just leave development mozmill tasks require the cli --server_root command to specify the root folder.

I don't think this feature is worth its potential performance and maintenance cost.
Attachment #8438476 - Attachment is obsolete: true
Attachment #8441395 - Flags: feedback?(jmaher)
Attachment #8441395 - Flags: feedback?(hskupin)
Comment on attachment 8441395 [details] [diff] [review]
WIP 2

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

I think this is good stuff!  Thanks for writing it and the little cleanup from last time.
Attachment #8441395 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 8441395 [details] [diff] [review]
WIP 2

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

I cannot share the concerns of you Andrei. While this may result in more parsing required, it should not cause lots of overhead. Manifest files are small and are most likely kept in the cache. But the benefits we can get as result out of this patch are huge in terms of manifest handling. No more code duplication at all for default values, and you can easily turn on/off features for specific folders.

In general this patch looks fine, but there are still some unclear items. Please see my inline comments.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +420,5 @@
>          return root
>  
>      ### methods for reading manifests
>  
> +    def _read(self, root, filename, defaults, parent_include=False):

Instead of 'parent_include' I would name it 'defaults_only', which IMO is more descriptive here.

@@ +462,5 @@
> +                    if self.strict:
> +                        raise IOError(message)
> +                    else:
> +                        sys.stderr.write("%s\n" % message)
> +                        continue

I don't like all that duplicated code with the include section. Make it maybe a local method.

@@ +464,5 @@
> +                    else:
> +                        sys.stderr.write("%s\n" % message)
> +                        continue
> +                self._ancestor_defaults.update(defaults)
> +                self._read(root, parent_file, defaults, True)

So I still don't understand this. You are traversing up the tree here, and are updating the parent's default on top of the formerly read manifest defaults. As we have said, lower-level manifest have to overwrite defaults set by the parent. This is still the other way around.

@@ +492,5 @@
>                  self._read(root, include_file, include_defaults)
>                  continue
>  
>              # otherwise an item
> +            data.update(self._ancestor_defaults)

Why can't this be done earlier?

::: testing/mozbase/manifestparser/tests/include/subinclude/level_2.ini
@@ +2,5 @@
> +x = level_2
> +
> +[test_2_1]
> +
> +[parent:level_1.ini]

The link to a parent manifest should not be added somewhere in the middle, but at the beginning of the file. IMO even before the DEFAULT section. Reason is that anything inside of parent gets evaluated first, before DEFAULT settings of this manifest are getting applied.
Attachment #8441395 - Flags: feedback?(hskupin) → feedback+
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Attached patch 3 (obsolete) — Splinter Review
I've moved the whole logic from _interpreting_ the file to _reading_ the file. When we reach the part where we actually load the tests we have all defaults computed.

We can have only 1 [parent:] include per file.
I've tried integrating this into the regular flow of reading files, but ultimately it's not worth it as we only care about 2 things: get all ancestors, and read the default values. There's a bit of code duplication but this is much better then trying to avoid this code when we do the usual stuff.

So there's a new function in read_ini which gets called _once_ if we have the [parent:%path%] include. This is a recursive method that calls itself for every new parent include it finds and returns the DEFAULT values.

I've used simple split's to read the relevant parts of the file. This can probably be improved.
Attachment #8441395 - Attachment is obsolete: true
Attachment #8444410 - Flags: feedback?(jmaher)
Attachment #8444410 - Flags: feedback?(hskupin)
There's a problem with the current inherited approach.
This does work fine, but for our intended case where we hold a path this doesn't work.

Lets say we're running some tests:
> mozmill -m mutt/mutt/tests/js/testWebServer/manifest.ini -b /Applications/FirefoxNightly.app/

When we reach mutt/mutt/tests/js/testWebServer/testMultipleLoads/test1.js we have the following info available:
> {'name': 'test1.js',
>  'server-root': '../data',
>  'here': '/Users/andrei.eftimie/work/mozilla/mozmill/src/mutt/mutt/tests/js/testWebServer/testMultipleLoads',
>  'manifest': '/Users/andrei.eftimie/work/mozilla/mozmill/src/mutt/mutt/tests/js/testWebServer/testMultipleLoads/manifest.ini',
>  'subsuite': '',
>  'expected': 'pass',
>  'path': '/Users/andrei.eftimie/work/mozilla/mozmill/src/mutt/mutt/tests/js/testWebServer/testMultipleLoads/test1.js',
>  'type': 'javascript',
>  'relpath': 'testMultipleLoads/test1.js'}

We've inherited `server-root` from `mutt/mutt/tests/js/manifest.ini` and we are 
now unable to compute the correct path. There's no way to know from where the
values came from with a simple obj merge.

We would have to keep a reference for _each_ key/value pair with the manifest file they came from...
Comment on attachment 8444410 [details] [diff] [review]
3

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

other than my one comment, the rest looks good.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +333,5 @@
> +
> +        # Read defaults
> +        obj = {}
> +        if '[DEFAULT]' in f:
> +            for item in f.split('[DEFAULT]')[1].split('[')[0].strip().splitlines():

I am not a fan of this split, split, split code here.  This appears that we need to read the parent defaults and then override them with defaults from the current file, right?  Isn't there a better way to do this using the existing parser for the default code?
Attachment #8444410 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 8444410 [details] [diff] [review]
3

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

Personally I do not like this move into read_ini. The code we have is totally not about reading the ini file, but about handling specific sections. read_ini is not aware of that. So this is not the right place for the code. Version 2 of the WIP was way better, the only problem I had was the way how default have been applied, which was the reversed order.

Regarding the path issue, we may be able to add a new specifier, which indicates that the value is of kind path. So we could easily construct the absolute path. But that would require the agreement from the mozbase drivers too. I would see benefits in that, e.g:

[DEFAULTS]
server_root = %../data/%
Attachment #8444410 - Flags: feedback?(hskupin) → feedback-
(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 8444410 [details] [diff] [review]
> 3
> 
> Review of attachment 8444410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Personally I do not like this move into read_ini. The code we have is
> totally not about reading the ini file, but about handling specific
> sections. read_ini is not aware of that. So this is not the right place for
> the code. 

Why? The [DEFAULT] variables are computed in read.ini
This is done _before_ we are looking at the sections.

If all is done right, when we start parsing each section we already have all DEFAULT values merged/flattened.
And since all we need the parent to do is to gather DEFAULT values from the ancestor tree it makes much more sense to do it _when_ we compute the actual DEFAULT values...
(In reply to Andrei Eftimie from comment #12)
> Why? The [DEFAULT] variables are computed in read.ini

Not sure what you mean with 'computed' here. But as the method name and its docstring say, the code here reads in an ini file. It doesn't do any evaluation or other complex operations. You simply get the content of the ini file. If you want to do more than that, you will have to enhance the code somewhere else.

> If all is done right, when we start parsing each section we already have all
> DEFAULT values merged/flattened.
> And since all we need the parent to do is to gather DEFAULT values from the
> ancestor tree it makes much more sense to do it _when_ we compute the actual
> DEFAULT values...

It's breaks the design IMO. Lets get also feedback from Andrew on that.
Flags: needinfo?(ahalberstadt)
Comment on attachment 8444410 [details] [diff] [review]
3

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

I'm a little confused by the specific use case presented in this bug.. if we have metadata that is the same for every test, then why is it in the manifests in the first place? Can't the test harness define it somewhere outside of the manifests? Or are there some tests that have a different doc root? Even in that case though, the default doesn't need to be in the manifests.

Anyway, imo the parent is the manifest that has the 'include' directive in it. This patch looks like you can specify a third manifest as the parent, which seems like it could be confusing. My gut feeling is that this is something that will increase complexity with little value added.. but if others think it would be useful I'm ok with it.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +332,5 @@
> +                             parent_file))
> +
> +        # Read defaults
> +        obj = {}
> +        if '[DEFAULT]' in f:

We can't do this because the section might not even be called "DEFAULT":
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py#299

Also I agree with Joel's comment below. I'd rather add some kind of "defaults" variable somewhere that stores these values (if we need to) rather than having to parse it out.
(In reply to Henrik Skupin (:whimboo) from comment #13)
> > If all is done right, when we start parsing each section we already have all
> > DEFAULT values merged/flattened.
> > And since all we need the parent to do is to gather DEFAULT values from the
> > ancestor tree it makes much more sense to do it _when_ we compute the actual
> > DEFAULT values...
> 
> It's breaks the design IMO. Lets get also feedback from Andrew on that.

I'm not 100% clear what you are asking.. but I definitely don't think we should be parsing manifest files before they are read. We should call read_ini first and then update the defaults later. So I think that means I agree with Henrik.
Flags: needinfo?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #15)
> I'm not 100% clear what you are asking.. but I definitely don't think we
> should be parsing manifest files before they are read. We should call
> read_ini first and then update the defaults later. So I think that means I
> agree with Henrik.

Yes, that's what I meant.

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> I'm a little confused by the specific use case presented in this bug.. if we
> have metadata that is the same for every test, then why is it in the
> manifests in the first place? Can't the test harness define it somewhere

The Mozmill tests are a bit different than the tests on mozilla-central, where you have definite test cases served by a HTTP server, and the test harness takes care of serving them automatically. So we are unbundled from mozilla-central and let the user store the tests where-ever they want on disk. They can execute them all, or by folder, or individual. So far we used -t to specify the test, but as we agreed on a while ago, we want to move to -m for running tests. So best is if the manifest can tell where the testcase data a test needs is located. Only in case the user wants to override the setting we make it possible via --server-root. To lower the complexity we really want that users can run the tests as easy as possible. So having the need to specify the docroot each time is cumbersome.
(In reply to Henrik Skupin (:whimboo) from comment #16)
> The Mozmill tests are a bit different than the tests on mozilla-central,
> where you have definite test cases served by a HTTP server, and the test
> harness takes care of serving them automatically. So we are unbundled from
> mozilla-central and let the user store the tests where-ever they want on
> disk. They can execute them all, or by folder, or individual. So far we used
> -t to specify the test, but as we agreed on a while ago, we want to move to
> -m for running tests. So best is if the manifest can tell where the testcase
> data a test needs is located. Only in case the user wants to override the
> setting we make it possible via --server-root. To lower the complexity we
> really want that users can run the tests as easy as possible. So having the
> need to specify the docroot each time is cumbersome.

I think I see what you mean, I guess this is ok then.
Blocks: 1028856
FInally an update here. This is (or hopefully should be) a fully working version.

From the feedback received thus far:
- This is based on the approach I took in WIP2 where the parsing is done in the parser itself, not in read_ini
- Based on comment 16 and comment 17 we now compute 'server-root' as an absolute path, relative to the manifest file. This is done in read_ini
Attachment #8444410 - Attachment is obsolete: true
Attachment #8475848 - Flags: review?(jmaher)
Attachment #8475848 - Flags: review?(hskupin)
Blocks: 1056037
Comment on attachment 8475848 [details] [diff] [review]
0004-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

there is a lot to wrap my mind around here.  Reading over this, I only have one question, walking through the code and the tests this seems pretty solid!

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +385,5 @@
>              local_dict['skip-if'] = "(%s) || (%s)" % (variables['skip-if'].split('#')[0], local_dict['skip-if'].split('#')[0])
>          variables.update(local_dict)
> +        if 'server-root' in variables:
> +            root = os.path.join(os.path.dirname(fp.name),
> +                                variables['server-root'])

where is fp defined?  I don't see it in the scope of this interpret_variables function.
Attachment #8475848 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #19)
> where is fp defined?  I don't see it in the scope of this
> interpret_variables function.

fp comes as the first argument of the read_ini method and if the actual file to be read. eg the manifest file we parse:
http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py#290-309
got it, thanks!
Comment on attachment 8475848 [details] [diff] [review]
0004-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

I like that patch. It looks kinda clean now, and mostly understandable. But add some more doc strings and comments as requested inline. For the tests, I would put them under testing/mozbase/manifestparser/tests/parent/, and not under 'include'. It's a different section type you want to test here. Further I miss tests for `server-root`.

Further please make sure that the documentation gets updated once the patch landed. Especially for server-root, which is a unique feature.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +386,5 @@
>          variables.update(local_dict)
> +        if 'server-root' in variables:
> +            root = os.path.join(os.path.dirname(fp.name),
> +                                variables['server-root'])
> +            variables['server-root'] = os.path.abspath(root)

I would add a comment which explains what's get done here and why.

@@ +416,5 @@
>  
>      ### methods for reading manifests
>  
> +    def _read(self, root, filename, defaults, defaults_only=False):
> +

Would you mind to add the docstring with parameter descriptions? It will help others to better understand the following code.

@@ +458,5 @@
>              subsuite = ''
>              if 'subsuite' in data:
>                  subsuite = data['subsuite']
>  
> +            # read the parent

change it to 'read the parent manifest if specified'

::: testing/mozbase/manifestparser/tests/include/subinclude/level_1.ini
@@ +1,2 @@
> +[DEFAULT]
> +x = level_1

You also wanna test server-root as variable name here because it is explicitly handled and modified. That way we can ensure that the abs path is correctly constructed. It would be best to have a separate test for it with sub dirs.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ +106,5 @@
>          self.assertEqual(buffer.getvalue().strip(),
>                           '[DEFAULT]\nfoo = bar\n\n[fleem]\nsubsuite = \n\n[include/flowers]\nblue = ocean\nred = roses\nsubsuite = \nyellow = submarine')
>  
> +    def test_parent_inheritance(self):
> +        """Illustrate how parent works"""

A better docstring for both test methods please.

@@ +115,5 @@
> +        # Parent manifest test should not be included
> +        self.assertEqual(parser.get('name'),
> +                         ['test_3'])
> +        self.assertEqual([(test['name'], os.path.basename(test['manifest'])) for test in parser.tests],
> +                         [('test_3', 'level_3.ini')])

What is this check for? If you want to test for known manifest files please don't use the test here. You already check that in the assert before. Maybe adding another comment?

@@ +137,5 @@
> +        # Parent manifest test should not be included
> +        self.assertEqual(parser.get('name'),
> +                         ['test_3'])
> +        self.assertEqual([(test['name'], os.path.basename(test['manifest'])) for test in parser.tests],
> +                         [('test_3', 'level_3_default.ini')])

Same as above.
Attachment #8475848 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #22)
> Comment on attachment 8475848 [details] [diff] [review]
> 0004-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch
> 
> Review of attachment 8475848 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like that patch. It looks kinda clean now, and mostly understandable. But
> add some more doc strings and comments as requested inline. For the tests, I
> would put them under testing/mozbase/manifestparser/tests/parent/, and not
> under 'include'. It's a different section type you want to test here.
> Further I miss tests for `server-root`.
> 
> Further please make sure that the documentation gets updated once the patch
> landed. Especially for server-root, which is a unique feature.
> 
> ::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
> @@ +386,5 @@
> >          variables.update(local_dict)
> > +        if 'server-root' in variables:
> > +            root = os.path.join(os.path.dirname(fp.name),
> > +                                variables['server-root'])
> > +            variables['server-root'] = os.path.abspath(root)
> 
> I would add a comment which explains what's get done here and why.
Done

> @@ +416,5 @@
> >  
> >      ### methods for reading manifests
> >  
> > +    def _read(self, root, filename, defaults, defaults_only=False):
> > +
> 
> Would you mind to add the docstring with parameter descriptions? It will
> help others to better understand the following code.
Done

> @@ +458,5 @@
> >              subsuite = ''
> >              if 'subsuite' in data:
> >                  subsuite = data['subsuite']
> >  
> > +            # read the parent
> 
> change it to 'read the parent manifest if specified'
Done

> ::: testing/mozbase/manifestparser/tests/include/subinclude/level_1.ini
> @@ +1,2 @@
> > +[DEFAULT]
> > +x = level_1
> 
> You also wanna test server-root as variable name here because it is
> explicitly handled and modified. That way we can ensure that the abs path is
> correctly constructed. It would be best to have a separate test for it with
> sub dirs.
I've changed to use subfolders for all tests.
I've added a new test for server-root, with new ini files for it, where I make 
3 checks:
- a regular variable is inherited 'as-is'
- there should not be any entry for the original 'server-root' value
- check the value of 'server-root' (the end of the path to match the expected folder)

> ::: testing/mozbase/manifestparser/tests/test_manifestparser.py
> @@ +106,5 @@
> >          self.assertEqual(buffer.getvalue().strip(),
> >                           '[DEFAULT]\nfoo = bar\n\n[fleem]\nsubsuite = \n\n[include/flowers]\nblue = ocean\nred = roses\nsubsuite = \nyellow = submarine')
> >  
> > +    def test_parent_inheritance(self):
> > +        """Illustrate how parent works"""
> 
> A better docstring for both test methods please.
Done

> @@ +115,5 @@
> > +        # Parent manifest test should not be included
> > +        self.assertEqual(parser.get('name'),
> > +                         ['test_3'])
> > +        self.assertEqual([(test['name'], os.path.basename(test['manifest'])) for test in parser.tests],
> > +                         [('test_3', 'level_3.ini')])
> 
> What is this check for? If you want to test for known manifest files please
> don't use the test here. You already check that in the assert before. Maybe
> adding another comment?
This check is making sure that parent: includes won't also append their respective tests.
When you do a regular include: you expect both the options and the tests to be parsed.
(ie in this case the list would contain ['test_1', 'test_2', 'test_3'])

For parent: we use the defaults_only argument which skips all tests.
Attachment #8475848 - Attachment is obsolete: true
Attachment #8483395 - Flags: review?(hskupin)
Updated because of bitrot.
Also found the documentation http://people.mozilla.org/~wlachance/mozbase-docs/manifestparser.html which lives in rts file, so I added a section on server-root.
Attachment #8483395 - Attachment is obsolete: true
Attachment #8483395 - Flags: review?(hskupin)
Attachment #8487137 - Flags: review?(hskupin)
(In reply to Andrei Eftimie from comment #24)
> Also found the documentation
> http://people.mozilla.org/~wlachance/mozbase-docs/manifestparser.html which
> lives in rts file, so I added a section on server-root.

That's not the actual documentation. It may be a left-over from testing before moving to readthedocs: http://mozbase.readthedocs.org/en/latest/

Can you also please push your patch to try? I haven't seen that it was run at least once so far.
Comment on attachment 8487137 [details] [diff] [review]
0006-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

Hard to review again given that no interdiff between those two patches are available on Bugzilla. :/ Beside the not updated location of the tests and some nits, I think we should be fine in the next round.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +385,5 @@
>          if 'skip-if' in local_dict and 'skip-if' in variables:
>              local_dict['skip-if'] = "(%s) || (%s)" % (variables['skip-if'].split('#')[0], local_dict['skip-if'].split('#')[0])
>          variables.update(local_dict)
>  
> +        # server-root is a os path declared relative to the manifest file.

nit: "an os path"

@@ +422,5 @@
>  
> +    def _read(self, root, filename, defaults, defaults_only=False):
> +        """
> +        internal recursive method for reading and parsing manifests.
> +        stores all found tests in self.tests

nit: Please start sentences with a capital letter.

@@ +423,5 @@
> +    def _read(self, root, filename, defaults, defaults_only=False):
> +        """
> +        internal recursive method for reading and parsing manifests.
> +        stores all found tests in self.tests
> +        - root : base path

For parameters please use the form: `:param %name%: %desc%` as you can see on other methods.

::: testing/mozbase/manifestparser/tests/include/level_1/level_1.ini
@@ +1,1 @@
> +[DEFAULT]

I mentioned in my last review that we should put the tests under `tests/parent` to separate them from the `include` tests. Looks like you missed to move them.

::: testing/mozbase/manifestparser/tests/include/level_1/level_1_server-root.ini
@@ +1,2 @@
> +[DEFAULT]
> +server-root = .

I would specify a sub-folder like `root` so that it is not located at the same level as the root manifest.

::: testing/mozbase/manifestparser/tests/include/level_1/level_2/level_3/level_3_server-root.ini
@@ +1,1 @@
> +[parent:../level_2_server-root.ini]

Do we need 3 levels here? IMO 2 should be enough.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ +107,5 @@
>                           '[DEFAULT]\nfoo = bar\n\n[fleem]\nsubsuite = \n\n[include/flowers]\nblue = ocean\nred = roses\nsubsuite = \nyellow = submarine')
>  
> +    def test_parent_inheritance(self):
> +        """
> +        Tests that inherited variables from parent includes properly propagate

If the full description doesn't fit into a single line, give a short summary in the first line, and put the detailed description in the 2nd and 3rd line. Also please use this wording: `Test` ... `parent manifests`

@@ +110,5 @@
> +        """
> +        Tests that inherited variables from parent includes properly propagate
> +        downstream
> +        """
> +

nit: no empty line here.

@@ +119,5 @@
> +        # Parent manifest test should not be included
> +        self.assertEqual(parser.get('name'),
> +                         ['test_3'])
> +        self.assertEqual([(test['name'], os.path.basename(test['manifest'])) for test in parser.tests],
> +                         [('test_3', 'level_3.ini')])

So as you say, you want to check if parent manifests are not including any referenced test. I don't see the need why the manifest itself has to be added here. Isn't it enough to test that parser.tests only contains test_3?

@@ +173,5 @@
> +        # we will not find anything for the original value
> +        self.assertEqual(parser.get('name', **{'server-root': '.'}), [])
> +
> +        # check that the path has expanded
> +        self.assertTrue(parser.get('server-root')[0].endswith('level_1'))

I would do a check for the realpath() by expanding the expected path appropriately.
Attachment #8487137 - Flags: review?(hskupin) → review-
In the meantime, try build with the previous patch:
https://tbpl.mozilla.org/?tree=Try&rev=eb1b00526fe5
Addressed issues from review, except the following:

> Do we need 3 levels here? IMO 2 should be enough.
Yes, I want to test a referenced file (via parent: which also has defaults_only)
which also includes a file via parent:

> So as you say, you want to check if parent manifests are not including any
> referenced test. I don't see the need why the manifest itself has to be added
> here. Isn't it enough to test that parser.tests only contains test_3?
I've took these from the test_include method:
http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/tests/test_manifestparser.py#57-61
I would keep the test for both the test file itself and for the referenced
manifest file.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1b53121fa574
(with all tests now)
Attachment #8487137 - Attachment is obsolete: true
Attachment #8493041 - Flags: review?(hskupin)
Comment on attachment 8493041 [details] [diff] [review]
0007-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +422,5 @@
>  
> +    def _read(self, root, filename, defaults, defaults_only=False):
> +        """
> +        Internal recursive method for reading and parsing manifests.
> +        stores all found tests in self.tests

nit: still missing capital letter.

::: testing/mozbase/manifestparser/tests/parent/level_1/level_1_server-root.ini
@@ +1,2 @@
> +[DEFAULT]
> +server-root = ../root

This causes test bustage because the folder does not exist. If git cannot handle empty directories you have to put a placeholder in it.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ +173,5 @@
> +        self.assertEqual(parser.get('name', **{'server-root': '../root'}), [])
> +
> +        # check that the path has expanded
> +        self.assertTrue(os.path.samefile(parser.get('server-root')[0],
> +                                         os.path.join(here, 'parent', 'root')))

As per the docs `samefile()` only works on Unix. We cannot make use of it.
Attachment #8493041 - Flags: review?(hskupin) → review-
Ah, didn't realise git won't include my empty folder. Added a dummy file.
Fixed the nit.

And change the path checking from sameFile to actually comparing both paths.

And another try run (with much less orange):
https://tbpl.mozilla.org/?tree=Try&rev=32458441f09d
Attachment #8493041 - Attachment is obsolete: true
Attachment #8493721 - Flags: review?(hskupin)
Andrei, for the try run I can see that some XPCshell tests are failing because a manifest file has not been found. Can you please check that your patch is not causing this?
I can't see how they could be related.

We have oranges:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48673003&tree=Try
> 272 ERROR TEST-UNEXPECTED-TIMEOUT | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_multiprovider.js | application timed out after 330 seconds with no output
> 273 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_multiprovider.js | application terminated with exit code -9

4 instances for Android like https://tbpl.mozilla.org/php/getParsedLog.php?id=48672450&tree=Try
> IOError: [Errno 2] No such file or directory: '/builds/slave/test/build/tests/xpcshell/tests/browser/components/loop/test/xpcshell/xpcshell.ini'

A crash on Android (flash?) https://tbpl.mozilla.org/php/getParsedLog.php?id=48672229&tree=Try
> PROCESS-CRASH | testFlingCorrectness | application crashed [@ 0x11e]
> PROCESS-CRASH | Automation Error: Missing end of test marker (process crashed?)
> Return code: 1
> 09-23 05:10:31.976 E/GeckoLinker( 4668): /data/data/com.adobe.flashplayer/lib/libysshared.so: Text relocations are not supported

An install error https://tbpl.mozilla.org/php/getParsedLog.php?id=48670736&tree=Try
> InstallError: Failed to install "/builds/slave/try-osx64_g-000000000000000000/build/testing/mozbase/mozinstall/tests/Installer-Stubs/firefox.dmg"
> make[1]: *** [test.py-run] Error 1
> make: *** [check] Error 2

And something failed because of missing updatesymbols (whatever these are) https://tbpl.mozilla.org/php/getParsedLog.php?id=48673362&tree=Try
> make: *** [uploadsymbols] Error 1
(In reply to Andrei Eftimie from comment #32)
> 4 instances for Android like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=48672450&tree=Try
> > IOError: [Errno 2] No such file or directory: '/builds/slave/test/build/tests/xpcshell/tests/browser/components/loop/test/xpcshell/xpcshell.ini'

It's more about this manifest. Does XPCshell make use of the manifestparser? Or were similiar failures seen the same day you issued your request? Maybe check the try run before and after.
Still missing an answer to my last comment.
Flags: needinfo?(andrei.eftimie)
Ugh, since that was last week I've requested a new try run:
https://tbpl.mozilla.org/?tree=Try&rev=3f2bf4cb1964

If those are still failing I'll investigate why that might be...
FYI next time please consider to only run those parts which are necessary. Each full try run takes away lot of machine resources we can safe for other jobs. Thanks.
Comment on attachment 8493721 [details] [diff] [review]
0008-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

The XPCShell failures in finding a manifest are still persistent. I do not see them for other try builds or on inbound. So those should be related to changes as done here. Please investigate why we are failing here.
Attachment #8493721 - Flags: review?(hskupin) → review-
Well this is interesting.

See another try run which appears to be green:
https://tbpl.mozilla.org/?tree=Try&rev=81e5552f723a 

For `Android 2.3 Opt` we have X1, X2, X3 as orange with my patch.

Let's compare the logs for X1.
Failing: https://tbpl.mozilla.org/php/getParsedLog.php?id=48672865&tree=Try
Passing: https://tbpl.mozilla.org/php/getParsedLog.php?id=49190873&tree=Try

> 05:19:30     INFO -  pushing /builds/slave/test/build/tests/xpcshell/tests
> 05:19:30     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/browser/components/loop/test/xpcshell/xpcshell.ini' does not exist
> 05:19:30     INFO -  Traceback (most recent call last):
> 05:19:30     INFO -    File "/builds/slave/test/build/tests/xpcshell/remotexpcshelltests.py", line 626, in <module>
> 05:19:30     INFO -      main()
> 05:19:30     INFO -    File "/builds/slave/test/build/tests/xpcshell/remotexpcshelltests.py", line 621, in main
> 05:19:30     INFO -      **options.__dict__):
> 05:19:30     INFO -    File "/builds/slave/test/build/tests/xpcshell/runxpcshelltests.py", line 1335, in runTests
> 05:19:30     INFO -      self.buildTestList()
> 05:19:30     INFO -    File "/builds/slave/test/build/tests/xpcshell/remotexpcshelltests.py", line 471, in buildTestList
> 05:19:30     INFO -      xpcshell.XPCShellTests.buildTestList(self)
> 05:19:30     INFO -    File "/builds/slave/test/build/tests/xpcshell/runxpcshelltests.py", line 798, in buildTestList
> 05:19:30     INFO -      mp.read(self.manifest)
> 05:19:30     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/manifestparser/manifestparser.py", line 569, in read
> 05:19:30     INFO -      self._read(here, filename, defaults)
> 05:19:30     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/manifestparser/manifestparser.py", line 492, in _read
> 05:19:30     INFO -      self._read(root, include_file, include_defaults)
> 05:19:30     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/manifestparser/manifestparser.py", line 450, in _read
> 05:19:30     INFO -      fp = open(filename)
> 05:19:30     INFO -  IOError: [Errno 2] No such file or directory: '/builds/slave/test/build/tests/xpcshell/tests/browser/components/loop/test/xpcshell/xpcshell.ini'

Versus:
> INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/browser/components/loop/test/xpcshell/xpcshell.ini' does not exist
> 03:43:53     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/dom/indexedDB/test/unit/xpcshell.ini' does not exist
> 03:43:53     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/dom/network/tests/unit_stats/xpcshell.ini' does not exist
> 03:43:53     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/dom/resourcestats/tests/xpcshell/xpcshell.ini' does not exist
> 03:43:53     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/modules/libmar/tests/unit/xpcshell.ini' does not exist
> 03:43:53     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/xpcshell.ini' does not exist
> 03:43:53     INFO -  Included file '/builds/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/update/tests/xpcshell.ini' does not exist
> 03:43:53     INFO -  Running tests 1-286/858
> 03:43:53     INFO -  INFO | Running tests sequentially.

It appears my changes made trying to include a missing file to throw (as they previously wouldn't).

I haven't changed any logic here, but we did refactor one method to take care of both include: and parent:, the fault is probably there.

I'll get this fixed.
Flags: needinfo?(andrei.eftimie)
Given that no other code is using the parent property of a manifest, it may be a silly side-effect for include. A look at the XPCShell code might give a clue how they make use of ManifestParser.
(In reply to Henrik Skupin (:whimboo) from comment #39)
> Given that no other code is using the parent property of a manifest, it may
> be a silly side-effect for include. A look at the XPCShell code might give a
> clue how they make use of ManifestParser.

I've actually found the problem.
As said before we moved some code from a loop into a helper method, and for none-found files it would have a continue statement if the file was not found. Seems I missed it.

Testing the fix atm.
Ah! Would be good to have a specific test for this situation then.
Fixed the issue, added a simple test for it.
(without the fix if throws exactly like the XPC failures).

Will add a new try run shortly.
Attachment #8493721 - Attachment is obsolete: true
Attachment #8497487 - Flags: review?(hskupin)
And try run is running: https://tbpl.mozilla.org/?tree=Try&rev=aa78d75241f3
Comment on attachment 8497487 [details] [diff] [review]
0009-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

The changes as done lately on this patch make sense, and looks like that functionality wise all is working fine now. Thanks!

I have some nits for remaining fixes. Please see my inline comments. With those done, we should be ready to get it landed.

::: testing/mozbase/docs/manifestparser.rst
@@ +146,5 @@
> +This variable is deemed a path and will be expanded into its absolute form.
> +
> +Because of the inheritant nature of the key/value pairs, if one requires a
> +system path, it must be absolute for it to be of any use in any included file.
> +

I checked the rst file and I think it would be good to give an example for server-root usage, as what we have for all the other stuff.

I would also like to see a paragraph and example added for parent.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +436,5 @@
> +            include_file = normalize_path(include_file)
> +            if not os.path.isabs(include_file):
> +                include_file = os.path.join(self.getRelativeRoot(here), include_file)
> +            if os.path.exists(include_file):
> +                return include_file

I would propose you leave what you had before, so we return a valid include file at the very end of this function.

@@ +441,5 @@
> +            message = "Included file '%s' does not exist" % include_file
> +            if self.strict:
> +                raise IOError(message)
> +            else:
> +                sys.stderr.write("%s\n" % message)

In case of a non-strict execution, you simply want to add a plain `return` statement here. That's similar to the `continue` statement, as used before inside the for loop.

Lets try to keep the logic for checking all negative situations first, and last but not least return the include file if valid.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ +180,5 @@
> +        """
> +        Test invalid path should not throw when not strict
> +        """
> +        manifest = os.path.join(here, 'include-invalid.ini')
> +        parser = ManifestParser(manifests=(manifest,), strict=False)

The code you have changed is part of the include: directive inside a manifest. This test may also cover a part of the problem, but not the specific one we have seen in the unit tests. What you want is a manifest with an invalid include directive.

Maybe you wanna add this test right next to test_include in that module.
Attachment #8497487 - Flags: review?(hskupin) → review-
Fixed all nits.

> The code you have changed is part of the include: directive inside a manifest.
> This test may also cover a part of the problem, but not the specific one we
> have seen in the unit tests. What you want is a manifest with an invalid
> include directive.
This is exactly what the test does, the manifest file exists, but it includes a non-existing file (exactly what the XPCshell tests were doing).

===
Since I've re-changed the logic a bit as requested, I've issued another try run (builds + formerly failing android xpcshell tests).

Seems everyone is switching to treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1e1d2e5f4607
Attachment #8497487 - Attachment is obsolete: true
Attachment #8498691 - Flags: review?(hskupin)
Comment on attachment 8498691 [details] [diff] [review]
0010-Bug-1023790-parent-include-for-manifest-files.-r-hsk.patch

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

r=me with the nit for the documentation fixed, and the try build passing.

::: testing/mozbase/docs/manifestparser.rst
@@ +124,5 @@
>  
>      [include:subdir/anothermanifest.ini]
>  
> +And reference parent manifests -- to inherit data even when only running a
> +sub manifest:

Maybe better: "And reference parent manifests to inherit keys and values from the DEFAULT section, without adding possible included tests.

::: testing/mozbase/manifestparser/tests/test_manifestparser.py
@@ +109,5 @@
> +    def test_invalid_path(self):
> +        """
> +        Test invalid path should not throw when not strict
> +        """
> +        manifest = os.path.join(here, 'include-invalid.ini')

Heh, sorry. I was looking in my local checkout instead of checking the new added file here. All fine, yes.
Attachment #8498691 - Flags: review?(hskupin) → review+
Try run all green. Updated the message as requested.
Attachment #8498691 - Attachment is obsolete: true
Attachment #8498772 - Flags: review+
Keywords: checkin-needed
I will get this landed, with the commit message updated later.
Keywords: checkin-needed
Oh, wait. As I have heard from Andrew or William we could even bump the version of Manifestparser if that is the only change since the last release. Can you please check that and clarify with one of them? That would safe us from another landing.
https://hg.mozilla.org/mozilla-central/rev/e845072de8df
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sprint]
You need to log in before you can comment on or make changes to this bug.