Closed Bug 1110837 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Mozbase, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: AndreeaMatei, Assigned: whimboo)

References

Details

(Keywords: regression, Whiteboard: [mozmill-2.1+])

Attachments

(1 file, 4 obsolete files)

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.
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.
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)
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.
Attached file testcase.zip (obsolete) —
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.
Attachment #8536603 - Flags: feedback?(hskupin)
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.
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.
Blocks: 970594
Component: Mozmill Tests → Mozmill
Depends on: 1056045
Product: Mozilla QA → Testing
Whiteboard: [mozmill-2.1+]
Version: Version 2 → unspecified
Attachment #8536603 - Flags: feedback?(hskupin)
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.
Severity: normal → major
Component: Mozmill → Mozbase
Summary: Tests are getting skipped due to some manifest issues → [manifestparser] If first test in the master manifest is disabled, all tests will be disabled
Attached file testcase.zip (obsolete) —
Updated testcase which exercises this problem.
Attachment #8536603 - Attachment is obsolete: true
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.
Blocks: 1023790
No longer depends on: 1056045
Keywords: regression
Summary: [manifestparser] If first test in the master manifest is disabled, all tests will be disabled → [manifestparser] If first include in the master manifest is disabled, all included manifests will be disabled
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?
Flags: needinfo?(jmaher)
One option could be to use 'default-only' also for read_ini() and return the DEFAULT section only.
Summary: [manifestparser] If first include in the master manifest is disabled, all included manifests will be disabled → [manifestparser] If parents are used, the first include/test in the master manifest is always used to determine defaults
Attached patch WIP v1 (obsolete) — Splinter Review
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #8538171 - Flags: feedback?(jmaher)
Attached patch Patch v1 (obsolete) — Splinter Review
Working patch with new tests included. So this really affects only those manifests which make use of parent and include both together.
Attachment #8537913 - Attachment is obsolete: true
Attachment #8538171 - Attachment is obsolete: true
Attachment #8538171 - Flags: feedback?(jmaher)
Attachment #8538362 - Flags: review?(jmaher)
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.
Attachment #8538362 - Flags: review?(jmaher) → review+
I think you answered your own questions with your patch.
Flags: needinfo?(jmaher)
(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
Attached patch Patch v1.1Splinter Review
Updated patch with removed white-space changes.
Attachment #8538362 - Attachment is obsolete: true
Attachment #8538674 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/524cd131ba1c
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/524cd131ba1c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.