920849 - Unable to run xpcshell tests from a duplicate manifest

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P2)

Description

[Dave Townsend [:mossop]]

Add-ons manager xpcshell tests are run twice to verify that everything works regardless of whether using packed or unpacked extensions. I used to be able to use "make -C toolkit/mozapps/extensions/test/xpcshell" and "make -C toolkit/mozapps/extensions/test/xpcshell-unpack". Apparently that has gone away now and I can't figure out how to run these tests by themselves.

Comment 1

[Gregory Szorc [:gps]]

We merged the directories by employing some #include magic in manifests.

I /thought/ the mach command supported running tests from a manifest file. Although, that would be all tests, not individual tests.

This kinda sucks for your workflow. We should fix this ASAP.

I have a solution in my head to fix this. It will blend nicely into a solution for bug 920193.

Comment 2

[Dave Townsend [:mossop]]

Yes, I can run all the tests in toolkit/mozapps/extensions, and it doubles them, but that's not really helpful for tracking failures. I also don't think we're logging which variant is failing

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 1: Write metadata for every test file

With this patch, we write out a JSON file holding metadata for every
test derived from manifests.

https://tbpl.mozilla.org/?tree=Try&rev=d3b6c5581993

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 2: Discover xpcshell tests through metadata, not filesystem

This patch doesn't work. But it does show the direction I'd like to go
in.

This patch creates a new helper class to interface with the
moz.build-derived test metadata created in part 1. That part is easy and
should be relatively free of controversy.

The "radical" part of this patch is changing the xpcshell-test mach
command to rely on this metadata as the sole source of test discovery.
Furthermore, it creates an in-memory TestManifest instance from the
saved test metadata (which itself was parsed from the original
xpcshell.ini manifests) and tries to pass this into the xpcshell test
runner for execution.

Unfortunately the code isn't working. I suspect I need to munge paths
inside the test metadata because the test runner is expecting tests to
be "pinned" to certain directories or something. I'll keep looking at
it.

I'm interested in feedback of this approach.

Eventually, we'd probably want the new helper class to emit TestManifest
instances which can be fed directly into a test harness. Down the road,
we can write some code that takes a TestManifest (complete with
annotated test flavor info) and invoke the appropriate test harness. We
could potentially replace the JSON file from part 1 with a master .ini
test manifest that combines tests from *every* harness. Lots of
possibilities here. Many of them beyond scope of this bug. But they
could influence the quick hack I'd like to land here.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 1: Write metadata for every test file

Added some additional "relpath" keys to the written JSON file to
facilitate less logic in downstream consumers.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Part 2: Discover xpcshell tests through metadata, not filesystem

With this patch, the mach xpcshell-test command will discover tests
through the all-tests.json file. There's a lot happening in this patch.
Let me explain.

Most of the code deals with loading all-tests.json and finding tests. I
wrote this as a standalone class because other test commands will want
to use it. I'm pretty sure the implementation for finding tests is no
worse off than before. Actually, it should be better. Now, you can type
just the name of a test (e.g. "test_load_modules.js") and it will
discover *all* tests having that string in their filename. Cool stuff.

To support running multiple versions of a test, I had to change
runxpcshelltests.py to accept a TestManifest instance. This is because
there was no good way to have it run the same test file with multiple
options without manifests. I could have written out a manifest file and
passed the path to it, but that's wasteful of I/O.

Somewhere Mossop wanted the ability to know which "version" of the
extensions tests is running (regular vs unpack). I didn't address that
in this patch. Will do that in another patch. But, I'd like to get
review on this first because that patch is heavily dependent on what we
do here.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 813124 [details] [diff] [review]
Part 2: Discover xpcshell tests through metadata, not filesystem

This has issues with cwd foo. Part 1 is fine to review, however!

Comment 8

[Dave Townsend [:mossop]]

Either this has regressed further or I was wrong. "mach toolkit/mozapps/extensions" only runs one set of the tests, not the others.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 813121 [details] [diff] [review]
Part 1: Write metadata for every test file

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

::: python/mozbuild/mozbuild/backend/common.py
@@ +55,5 @@
> +    def __init__(self, config):
> +        self.config = config
> +        self.topsrcdir = mozpath.normpath(config.topsrcdir)
> +
> +        self.tests_by_path = {}

I would probably just use a defaultdict(list) here.

Comment 10

[(not currently active) Ted Mielczarek]

(Sorry for the review lag there.)

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67e2829b7706

Dave: sorry for the lag in landing here. You wouldn't believe how many distractions I've had in the past few weeks. It's not fair to you that you can't run tests you used to be able to. I'll try to brush part 2 up ASAP.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Part 2: Discover xpcshell tests through metadata, not filesystem

At the heart of this patch is code to resolve tests from the
all-tests.json file. You pass in some arguments/filters and it spits out
test files that match. This new code will essentially form the "test
lookup" API. This is all the code inside the mozbuild package.

The first consumer of this API is the xpcshell harness.

I could probably spend a lot more time writing tests. But, I want to get
this out the door so Dave can run tests.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/67e2829b7706

Comment 14

[Joshua Cranmer [:jcranmer]]

Comment on attachment 813121 [details] [diff] [review]
Part 1: Write metadata for every test file

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

::: python/mozbuild/mozbuild/backend/common.py
@@ +62,5 @@
> +        t = dict(t)
> +        t['flavor'] = flavor
> +
> +        path = mozpath.normpath(t['path'])
> +        assert path.startswith(self.topsrcdir)

Quick, can you spot the bug here that breaks comm-central?

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Fix for trees with more than one topsrcdir

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 821008 [details] [diff] [review]
Fix for trees with more than one topsrcdir

  https://hg.mozilla.org/integration/mozilla-inbound/rev/06880eeb81a0

Comment 17

[Javi Rueda :javirid]

Thank you Joshua. I had been unable to build TB until discovered this bug. This was the same day I upgraded to OS X 10.9 so I tried tons of solutions before trying that one.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06880eeb81a0

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 820706 [details] [diff] [review]
Part 2: Discover xpcshell tests through metadata, not filesystem

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

Sorry for the exceptionally long delay. :-/

Not r+ing for the _init question. If you can answer that satisfactorily I can probably r+.

::: python/mozbuild/mozbuild/base.py
@@ +508,4 @@
>              topobjdir=self.topobjdir)
>  
> +        if hasattr(c, '_init'):
> +            c._init()

I don't really understand why this needs to be split out into a separate method. Can you explain?

::: python/mozbuild/mozbuild/testing.py
@@ +56,5 @@
> +
> +        for path in sorted(self._tests_by_flavor.get(flavor, [])):
> +            yield self._tests_by_path[path]
> +
> +    def resolve_tests(self, what=None, flavor=None, under_path=None):

"what" is kind of a crappy variable name. This method feels like it does too many things, but I'm not sure how I'd simplify it.

@@ +97,5 @@
> +                    continue
> +
> +                candidates.extend(tests)
> +
> +            for test in fltr(candidates):

Feels like you could do this without making an intermediate list, but eh.

Comment 20

[Gregory Szorc [:gps]]

Attachment: Part 2: Discover xpcshell tests through metadata, not filesystem

Addressed review feedback.

Comment 21

[Gregory Szorc [:gps]]

I'm going to file a separate bug for the printing issue. IMO it needs to be tracked separately because it will change the output of the test runner and this may break orangefactor and other automation. I don't want that to get conflated with the intent of this bug.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 8356848 [details] [diff] [review]
Part 2: Discover xpcshell tests through metadata, not filesystem

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

This looks good, sorry for the delay.

::: python/mozbuild/mozbuild/testing.py
@@ +73,5 @@
> +        If ``under_path`` is a string, it will be used to filter out tests that
> +        aren't in the specified path prefix relative to topsrcdir or the
> +        test's installed dir.
> +
> +        If ``flavor`` is a string, we will filter returned tests to only be the

This is the only part of the docstring that uses "we", change it to be consistent?

::: testing/xpcshell/mach_commands.py
@@ +85,4 @@
>  
> +        # Dynamically write out a manifest holding all the discovered tests.
> +        manifest = TestManifest()
> +        manifest.tests.extend(tests)

Feels like it'd be nice to just use the existing manifest for simple cases (running a directory of tests with a single manifest), but this code looks pretty simple right now, so maybe it's not worthwhile.

@@ -94,5 @@
>              'interactive': interactive,
>              'keep_going': keep_going,
>              'shuffle': shuffle,
>              'sequential': sequential,
> -            'test_dirs': xpcshell_dirs,

Wonder if we could just remove the non-manifest support from runxpcshelltests.py at this point?

Comment 23

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> Wonder if we could just remove the non-manifest support from
> runxpcshelltests.py at this point?

We probably could!

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af1f556b0c09

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/af1f556b0c09