Closed
Bug 920849
Opened 11 years ago
Closed 11 years ago
Unable to run xpcshell tests from a duplicate manifest
Categories
(Firefox Build System :: General, defect, P2)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla29
People
(Reporter: mossop, Assigned: gps)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:P-])
Attachments
(3 files, 4 obsolete files)
9.62 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
20.36 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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
Reporter | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
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
Attachment #810708 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gps
Updated•11 years ago
|
Attachment #810708 -
Flags: review?(ted) → review+
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Added some additional "relpath" keys to the written JSON file to
facilitate less logic in downstream consumers.
Attachment #813121 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Attachment #810708 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
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.
Attachment #813124 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Attachment #810788 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #813121 -
Flags: review?(ted) → review+
Comment 10•11 years ago
|
||
(Sorry for the review lag there.)
Assignee | ||
Comment 11•11 years ago
|
||
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.
Status: NEW → ASSIGNED
Flags: in-testsuite+
Whiteboard: [leave open]
Assignee | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #813124 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
Comment 14•11 years ago
|
||
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•11 years ago
|
||
Attachment #821008 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #821008 -
Flags: review?(gps) → review+
Comment 16•11 years ago
|
||
Comment on attachment 821008 [details] [diff] [review]
Fix for trees with more than one topsrcdir
https://hg.mozilla.org/integration/mozilla-inbound/rev/06880eeb81a0
Attachment #821008 -
Flags: checkin+
Comment 17•11 years ago
|
||
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•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [leave open] → [leave open][Australis:P-]
Comment 19•11 years ago
|
||
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.
Attachment #820706 -
Flags: review?(ted)
Assignee | ||
Comment 20•11 years ago
|
||
Addressed review feedback.
Attachment #8356848 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Attachment #820706 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
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.
Updated•11 years ago
|
Priority: P1 → P2
Comment 22•11 years ago
|
||
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?
Attachment #8356848 -
Flags: review?(ted) → review+
Assignee | ||
Comment 23•11 years ago
|
||
(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!
Assignee | ||
Comment 24•11 years ago
|
||
Whiteboard: [leave open][Australis:P-] → [Australis:P-]
Comment 25•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•