Closed Bug 920849 Opened 6 years ago Closed 6 years ago

Unable to run xpcshell tests from a duplicate manifest


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



(Not tracked)



(Reporter: mossop, Assigned: gps)


(Blocks 1 open bug)


(Whiteboard: [Australis:P-])


(3 files, 4 obsolete files)

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.
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.
OS: Windows 7 → All
Priority: -- → P1
Hardware: x86_64 → All
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
With this patch, we write out a JSON file holding metadata for every
test derived from manifests.
Attachment #810708 - Flags: review?(ted)
Assignee: nobody → gps
Attachment #810708 - Flags: review?(ted) → review+
This patch doesn't work. But it does show the direction I'd like to go

This patch creates a new helper class to interface with the 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

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.
Added some additional "relpath" keys to the written JSON file to
facilitate less logic in downstream consumers.
Attachment #813121 - Flags: review?(ted)
Attachment #810708 - Attachment is obsolete: true
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 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.
Attachment #813124 - Flags: review?(ted)
Attachment #810788 - Attachment is obsolete: true
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!
Attachment #813124 - Flags: review?(ted)
Either this has regressed further or I was wrong. "mach toolkit/mozapps/extensions" only runs one set of the tests, not the others.
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/
@@ +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.
Attachment #813121 - Flags: review?(ted) → review+
(Sorry for the review lag there.)

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.
Flags: in-testsuite+
Whiteboard: [leave open]
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.
Attachment #820706 - Flags: review?(ted)
Attachment #813124 - Attachment is obsolete: true
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/
@@ +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?
Attachment #821008 - Flags: review?(gps) → review+
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.
Blocks: 938019
Whiteboard: [leave open] → [leave open][Australis:P-]
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/
@@ +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/
@@ +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.
Attachment #820706 - Flags: review?(ted)
Addressed review feedback.
Attachment #8356848 - Flags: review?(ted)
Attachment #820706 - Attachment is obsolete: true
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.
Blocks: 957380
Priority: P1 → P2
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/
@@ +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/
@@ +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 at this point?
Attachment #8356848 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> Wonder if we could just remove the non-manifest support from
> at this point?

We probably could!
Whiteboard: [leave open][Australis:P-] → [Australis:P-]
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Blocks: 964102
Depends on: 968581
Blocks: 969524
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.