658671 - [manifestparser] Manifest format doesn't support nested includes

Status: RESOLVED WONTFIX

(Testing :: Mozbase, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

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

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 ?

Comment 2

[Jeff Hammel]

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?)

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Jeff Hammel]

Do you have a diff I could take a look at?

Comment 5

[Mark Banner (:standard8)]

Attachment: Attempted patch

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.

Comment 6

[Jeff Hammel]

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 7

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

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.

Comment 8

[Jeff Hammel]

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

And if we don't, we should

Comment 9

[Javi Rueda :javirid]

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.

Comment 10

[Javi Rueda :javirid]

Correction to nyself. Test aren't run if host OS is different from Windows.

Comment 11

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

(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.

Comment 12

[Jeff Hammel]

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).

Comment 13

[Jeff Hammel]

(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

Comment 14

[Jeff Hammel]

(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.

Comment 15

[Jeff Hammel]

(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?

Comment 16

[Mark Banner (:standard8)]

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.

Comment 17

[Jeff Hammel]

Attachment: bug-658671.diff

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

Comment 18

[Jeff Hammel]

Attachment: quickfix.diff

to be applied atop attachment 791556 [details] [diff] [review]

Comment 19

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

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 ./?

Comment 20

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

Comment on attachment 791564 [details] [diff] [review]
quickfix.diff

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

should normalize_path() do this?

Comment 21

[Jeff Hammel]

(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.

Comment 22

[Jeff Hammel]

(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 .

Comment 23

[Mark Banner (:standard8)]

Per bug 718408 we should have wontfixed this a while ago.