Open Bug 658671 Opened 9 years ago Updated 8 months ago

[manifestparser] Manifest format doesn't support nested includes

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: standard8, Unassigned)

References

Details

(Whiteboard: [mozbase])

Attachments

(3 files)

If I want to do:

/mailnews/globalmanifest.ini -> (includes) -> /mailnews/.../submanifest.ini

and then include globalmanifest.ini from another file, the manifest parser fails silently and doesn't run the tests.

This means I can't include the same manifest file in different places e.g. for more than one app, or so that a group of manifest files have one inclusion location.
it could be that you are doing it incorrectly.

For example, we have this in the tree now:
testing/xpcshell/xpcshell.ini
[include:chrome/test/unit/xpcshell.ini]
[include:chrome/test/unit_ipc/xpcshell.ini]


Then I tested doing this:
testing/xpcshell/xpcshell.ini
[include:chrome/test/xpcshell.ini]

chrome/test/xpcshell.ini
[include:unit/xpcshell.ini]
[include:unit_ipc/xpcshell.ini]

this worked just fine.

The include file is relative to the current directory, so it could be that you are not referencing correct paths in the include directive ?
I'm not really sure what is desired here.  I fixed one bug regarding nested includes, so you might try with the tip of http://hg.mozilla.org/automation/ManifestDestiny . If that doesn't work, could you please include a non-working example of what is desired? (and possibly better explain what is desired?)
Ok, I've now investigated this a bit more.

Here's the relevant file locations:

mailnews/addrbook/tests/unit/ - contains an xpcshell.ini and some tests.
mailnews/xpcshell.ini - sub-manifest file for the mailnews xpcshell tests.
mail/test/xpcshell.ini - our master xpcshell.ini, overrides the mozilla-central version.
<objdir>/mozilla/_tests/xpcshell/ - this is where the tests get installed.

In mail/test/xpcshell.ini, I put one line:

[include:mailnews/xpcshell.ini]

In mailnews/xpcshell.ini, here's a subset of what I put:

[include:addrbook/test/unit/xpcshell.ini]

When I compile this, xpccheck.py complains that it can't find mailnews/addrbook/tests/unit/xpcshell.ini in mail/test/xpcshell.ini.

To "fix" that, I change mailnews/xpcshell.ini to read:

[include:mailnews/addrbook/test/unit/xpcshell.ini]

It compiles fine, but then doesn't run those tests - no errors, nothing.

If I then edit the <objdir> version of mailnews/xpcshell.ini to drop the mailnews/ then the tests run fine.
Blocks: 718408
No longer blocks: 658666
Component: New Frameworks → Mozbase
QA Contact: new-frameworks → mozbase
Whiteboard: [mozbase]
Do you have a diff I could take a look at?
Attached patch Attempted patchSplinter Review
This is what I'm trying to do (applies against comm-central). At the moment it gives me:

TEST-UNEXPECTED-FAIL | xpccheck | directory /Users/moztest/comm/main/src/mailnews/test is missing from master xpcshell.ini file /Users/moztest/comm/main/src/mail/test/xpcshell.ini

which I don't quite understand.
We should probably fix this ASAP.  I don't really understand the issue, TBH, but I haven't had a chance to repro yet.  A minimal test case would help
Comment on attachment 669452 [details] [diff] [review]
Attempted patch

>+++ b/mailnews/test/xpcshell.ini
>@@ -0,0 +1,15 @@
>+[include:mailnews/addrbook/test/unit/xpcshell.ini]

I don't think that this will work because in this case the Manifestparser will look for a manifest file located under mailnews/test/mailnews/addrbook/test/unit/xpcshell.ini. So what you probably want and which should work is an entry like:

>+[include:../addrbook/test/unit/xpcshell.ini]

It's not tested but AFAIR we support relative paths.
> It's not tested but AFAIR we support relative paths.

And if we don't, we should
Sorry if I disturb the flow in this bug comments. I am working on bug 767655 and I needed to create some tests that ran if the OS is Linux. As I am new into this, I am researching a little bit, as I need to modify that global xpcshell.ini -same problem as in comment 5 from Mark-.

Excepting that /steel/mac directory, the other ones seems too be not dependant on Mac.

In http://mxr.mozilla.org/comm-central/source/mail/base/test/unit/xpcshell.ini, in fact, tests are run if the OS is different from Windows, which includes Mac and Linux.

What I meant is if the global xpcshell.ini could be modified so other tests are run whatever was the host OS, it could simplify THIS bug.
Correction to nyself. Test aren't run if host OS is different from Windows.
(In reply to Javi Rueda from comment #9)
> Sorry if I disturb the flow in this bug comments. I am working on bug 767655
> and I needed to create some tests that ran if the OS is Linux. As I am new
> into this, I am researching a little bit, as I need to modify that global
> xpcshell.ini -same problem as in comment 5 from Mark-.

This is totally unrelated to this bug. If you want to get the ManifestParser information please use the dev.tools mailing list.
Since we're actually actively developing mozbase, we should fix this one of these days (and yes, I realize the irony inherent in prioritizing to prioritize).
(In reply to Jeff Hammel [:jhammel] from comment #8)
> > It's not tested but AFAIR we support relative paths.
> 
> And if we don't, we should

I *think* this means that [include:*path*] doesn't work if you have, e.g., ../foo/../bar or some other combination of e.g. relative paths, dots, etc.  We should ensure that we have tests for this; if we can test a parallel case, then i'm fine closing
Flags: needinfo?(jhammel)
Summary: Manifest format doesn't support nested includes → [manifestdestiny] Manifest format doesn't support nested includes
(In reply to Jeff Hammel [:jhammel] from comment #13)
> (In reply to Jeff Hammel [:jhammel] from comment #8)
> > > It's not tested but AFAIR we support relative paths.
> > 
> > And if we don't, we should
> 
> I *think* this means that [include:*path*] doesn't work if you have, e.g.,
> ../foo/../bar or some other combination of e.g. relative paths, dots, etc. 
> We should ensure that we have tests for this; if we can test a parallel
> case, then i'm fine closing

Mark, can you speak to the above?  I'm planning on writing/checking for tests to test my understanding of what's going on.  If you have STR...that would help; a failing test, even better.
Flags: needinfo?(jhammel) → needinfo?(mbanner)
(In reply to Jeff Hammel [:jhammel] from comment #13)
> (In reply to Jeff Hammel [:jhammel] from comment #8)
> > > It's not tested but AFAIR we support relative paths.
> > 
> > And if we don't, we should
> 
> I *think* this means that [include:*path*] doesn't work if you have, e.g.,
> ../foo/../bar or some other combination of e.g. relative paths, dots, etc. 
> We should ensure that we have tests for this; if we can test a parallel
> case, then i'm fine closing

Sorry, didn't mean to cancel the [needinfo] to myself?
Flags: needinfo?(jhammel)
Unfortunately I don't have anything more than what I mentioned in comment 0 and comment 3. Additionally, we're using automatically generated manifests in comm-central now, so this doesn't affect us any more.
Flags: needinfo?(mbanner)
Attached patch bug-658671.diffSplinter Review
While I'm not sure if I reproed this exact bug, I did produce a failing test in my attempt:

(mozbase)│./test_manifestparser.py TestManifestparser.test_include_relative
F
======================================================================
FAIL: test_include_relative (__main__.TestManifestparser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./test_manifestparser.py", line 119, in test_include_relative
    self.assertEqual(flowers, parser.tests[1]['path'])
AssertionError: '/home/jhammel/mozbase/src/mozbase/manifestdestiny/tests/include-relative/flowers.js' != '/home/jhammel/mozbase/src/mozbase/manifestdestiny/tests/include-relative/./flowers.js'

A strategically placed os.path.normpath should fix this
Attachment #791556 - Flags: feedback?(jmaher)
Flags: needinfo?(jhammel)
Attached patch quickfix.diffSplinter Review
to be applied atop attachment 791556 [details] [diff] [review]
Attachment #791564 - Flags: feedback?(jmaher)
Comment on attachment 791556 [details] [diff] [review]
bug-658671.diff

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

please document these tests so it is clearer that we are testing relative paths.

::: manifestdestiny/manifestparser/manifestparser.py
@@ +272,3 @@
>          fp = file(fp)
> +    else:
> +        filename = getattr(fp, 'name', str(fp))

I don't understand this

::: manifestdestiny/tests/include-relative/bar/bar.ini
@@ +1,5 @@
> +[DEFAULT]
> +foo = fleem
> +
> +[bar.js]
> +[include:.././included.ini]

why the .././, is this testing a specific parsing issue?

::: manifestdestiny/tests/include-relative/foo/manifest.ini
@@ +1,1 @@
> +[./test.js]

why the ./?
Attachment #791556 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 791564 [details] [diff] [review]
quickfix.diff

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

should normalize_path() do this?
Attachment #791564 - Flags: feedback?(jmaher) → feedback+
(In reply to Joel Maher (:jmaher) from comment #20)
> Comment on attachment 791564 [details] [diff] [review]
> quickfix.diff
> 
> Review of attachment 791564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> should normalize_path() do this?

That is a good question.  I'm not sure?  normalize_path is a bit misleading, as it normalizes -> UNIX style paths vs normalizes e.g. bar/./foo -> bar/foo . I think it should probably utilize os.path.normpath and will include this in the patch to land, when I post that.
(In reply to Joel Maher (:jmaher) from comment #19)
> Comment on attachment 791556 [details] [diff] [review]
> bug-658671.diff
> 
> Review of attachment 791556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please document these tests so it is clearer that we are testing relative
> paths.

Will do when posting final patch.

> ::: manifestdestiny/manifestparser/manifestparser.py
> @@ +272,3 @@
> >          fp = file(fp)
> > +    else:
> > +        filename = getattr(fp, 'name', str(fp))
> 
> I don't understand this

"If the file-like object has a `name` attribute, use that as the filename for reporting purposes.  Otherwise, use the object's `str` representation (e.g. '<StringIO.StringIO instance at 0x94cf50c>')."

I will include this comment in the final patch

> ::: manifestdestiny/tests/include-relative/bar/bar.ini
> @@ +1,5 @@
> > +[DEFAULT]
> > +foo = fleem
> > +
> > +[bar.js]
> > +[include:.././included.ini]
> 
> why the .././, is this testing a specific parsing issue?
> 
> ::: manifestdestiny/tests/include-relative/foo/manifest.ini
> @@ +1,1 @@
> > +[./test.js]
> 
> why the ./?

The point of this bug is to test relative paths in manifests.  I don't know if I reproduced the original issue, but the [./test.js] does result in an error: comment 17 .
Summary: [manifestdestiny] Manifest format doesn't support nested includes → [manifestparser] Manifest format doesn't support nested includes
You need to log in before you can comment on or make changes to this bug.