1110837 - [manifestparser] If parents are used, the first include/test in the master manifest is always used to determine defaults

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andreea Matei [:AndreeaMatei]]

Today, half of the runs on nightly for functional ended up skipping all tests:

We found an issue that we miss to remove a test from a manifest in bug 1093598, fixed that, but it still skips all.
We see 
06:34:02 Included file 'c:\jenkins\workspace\mozilla-central_functional\data\mozmill-tests\firefox\tests\functional\restartTests\testAddons_uninstallExtension\manifest.ini' does not exist

Looking now to see where that was removed as the test doesn't seem to exist anymore:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/restartTests/

But still, that doesn't fixes the skip issue. Are just some things we noticed now. 
Teodor believes it's related to bug 1096178 where we skipped a test in the manifest, but at that time it was the second in the list. Today we moved the first test in testAddons, and so this one became the first test. Somehow manifest  behaves wrong. 

http://mm-ci-production.qa.scl3.mozilla.com:8080/job/mozilla-central_functional/29640/console

We'll fix these in the related bugs but we want to have this for tracking and fixing the real issue. Filed as mozmill tests bug, might need to move.

Comment 1

[Andreea Matei [:AndreeaMatei]]

So we fixed the manifests were they were missed and fixed temporarily the issue in bug 1096178 by disabling inside the folder's manifest, the one with test1.js, test2.js.
We'll investigate more on Monday, I believe it's a wrong behavior of the manifest files.

Comment 2

[Andrei Eftimie]

This might be a manifestparser bug. 
I've seen this once: had a disabled inclusion test with the first test named test1.js
(this was a local instance when I was testing something else)

Somehow for the particular folder when the above happened I got the whole folder skipped.
Manifestparser didn't return any more tests. (If I remember correctly)

Comment 3

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

Andreea, if it is possible please try to find a minimized testcase and attach all necessary files as a zip archive. I could have a look at it then.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Attachment: testcase.zip

Added testcase, so we have 2 folders, each containing one test. The global manifest has the disabled line on the first folder, but the behavior skips all following folders too. This used to work before, we were using it in restartTests.

If the disabled line is not at the first item in the manifest, all works well.

Comment 5

[Andreea Matei [:AndreeaMatei]]

andreeamatei@sl1671-216:~/Downloads/testcase$ mozmill -m manifest.ini -b /Applications/FirefoxNightly.app/
TEST-SKIPPED | testFromFirstFolder.js | Bug 123 - "With this, all tests are getting skipped"
TEST-SKIPPED | testFromSecondFolder.js | Bug 123 - "With this, all tests are getting skipped"
RESULTS | Passed: 0
RESULTS | Failed: 0
RESULTS | Skipped: 2

testFromSecondFolder.js should have run.

Comment 6

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

This happens because the usage of [parent:] in those sub manifest files. We do not use a version of manifestparser, which can handle that yet. Andrei's patch on bug 1056045 has to be landed first.

Because of that the manifest file as referenced is not a valid path, and mozmill aborts. It should be fixed with Mozmill 2.1 soon.

Comment 7

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

The change I mentioned before have not been fixed that problem. It still persists, and indeed looks like a real manifestparser bug. I have a small application locally which perfectly exercises this problem. I will try to figure out what's wrong, given that this is a huge issue for us in Mozmill tests.

Comment 8

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

Attachment: testcase.zip

Updated testcase which exercises this problem.

Comment 9

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

This has been introduced with manifestparser 0.7. The only change as landed there was bug 1023790. So it should be a fallout from that.

Comment 10

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

Ok, so the problem here is iterating over the parents. The parameter 'defaults_only' is not doing what we expect from it. So lets take this parent manifest:

> [DEFAULT]
> server-root = data
>
> [include:testFirstFolder/manifest.ini]
> disabled = Bug 123 - "With this, all tests are getting skipped"
> [include:testSecondFolder/manifest.ini]

Once it is read-in we DO NOT get a separate section for `DEFAULT`, but only entries for each `include:`. We are iterating over those sections, and in case of default-only we break out for the first entry. This will always be testFirstFolder with the disabled flag set. Even when this file is read as parent for testSecondFolder!! As result the disabled flag is present everywhere!

I'm not sure if it would make sense the let a DEFAULT section be returned from the read ini method. Independent from any test or include referenced in this manifest. But this seems to be the only way to really see the global defaults.

Joel, what do you think?

Comment 11

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

One option could be to use 'default-only' also for read_ini() and return the DEFAULT section only.

Comment 12

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

Attachment: WIP v1

A work in progress patch to fix this problem. All existing tests are passing, and a new one still has to be added. I will do it tomorrow. Maybe I can get an early feedback.

Comment 13

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

Attachment: Patch v1

Working patch with new tests included. So this really affects only those manifests which make use of parent and include both together.

Comment 14

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ddb954ba5b53

Comment 15

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

Comment on attachment 8538362 [details] [diff] [review]
Patch v1

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

these patches require a lot of re-reading, after a couple passes, this makes sense- I am having a hard time determining if we have other conditions where this could be problematic.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +425,5 @@
>  
>      ### methods for reading manifests
>  
> +    def _read(self, root, filename, defaults, defaults_only=False,
> +              parentmanifest=None):

how did this line change?  just whitespace?

@@ +518,5 @@
>  
>              # determine the path
>              path = test.get('path', section)
>              _relpath = path
> +            if '://' not in path:  # don't futz with URLs

why is this line changed?  we added an extra space before #, not really necessary

@@ +559,5 @@
> +        # defaults section of the manifest without interpreting variables
> +        if defaults_only and not parent_section_found:
> +            sections = read_ini(fp=fp, variables=defaults, defaults_only=True,
> +                                strict=self.strict)
> +            (section, self._ancestor_defaults) = sections[0]

interesting, good find.

Comment 16

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

I think you answered your own questions with your patch.

Comment 17

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

(In reply to Joel Maher (:jmaher) from comment #15)
> these patches require a lot of re-reading, after a couple passes, this makes
> sense- I am having a hard time determining if we have other conditions where
> this could be problematic.

So far this all depend on parent and include, where we include other manifest data in both directions. As long as parent is not used the newly added code should not have introduced a regression. I tested a lot locally but no other combination failed. So I think we should be safe here.

> ::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
> @@ +425,5 @@
> >  
> >      ### methods for reading manifests
> >  
> > +    def _read(self, root, filename, defaults, defaults_only=False,
> > +              parentmanifest=None):
> 
> how did this line change?  just whitespace?

Yes, I will revert those whitespace changes, and maybe come up with white-space only fix when I find the time. 

> > +            if '://' not in path:  # don't futz with URLs
> 
> why is this line changed?  we added an extra space before #, not really
> necessary

Same here. The extra space is necessary to make it PEP8 conform. It requires at least 2 spaces:
https://www.python.org/dev/peps/pep-0008/#inline-comments

Comment 18

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

Attachment: Patch v1.1

Updated patch with removed white-space changes.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/524cd131ba1c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/524cd131ba1c