Closed Bug 1317970 Opened 3 years ago Closed 3 years ago

Create manifestparser manifests for |mach python-test|

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(4 files)

Currently we define python unittests directly in moz.build files under the PYTHON_UNITTESTS variable. Instead, I'd like to create python.ini manifests and define them in PYTHON_MANIFESTS. There are a bunch of advantages to this:

1. We can use existing concepts like "subsuite" to split the tests into different tasks. This will make removing tests out of "make check" one chunk at a time much easier. I think subsuites have an advantage over using file paths for this: there will be a default set (tests with no subsuite) which will ensure we can't accidentally add new tests that don't get run in any tasks.

2. We can skip-if/fail-if tests with the normal mozinfo.json. Granted, this is still only at the file level, but at least the option is there.

3. We no longer need to special case python unittests in the build system. They would just follow the normal manifestparser based code paths.
Component: General → Build Config
Product: Testing → Core
Yeah, this is pretty sensible. I thought about this in the context of splitting them into two types (for jobs that need an objdir) and hadn't settled on a solution, but this will be fine.
bikeshed: I suggest `PYTHON_TEST_MANIFESTS` or `PYTHON_UNITTEST_MANIFESTS`.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Yeah, this is pretty sensible. I thought about this in the context of
> splitting them into two types (for jobs that need an objdir) and hadn't
> settled on a solution, but this will be fine.

I also like subsuites because we can split them into N types. For example, the mozbase tests can exist in their own task that only runs when a file under testing/mozbase is changed.

I agree PYTHON_UNITTEST_MANIFESTS is better.
Fun anecdote.. Previously the entire mozbase test suite was being treated as a single test by PYTHON_UNIT_TESTS. This means we weren't able to parallelize them with the newly landed -j flag in |mach python-test|.

After fixing that, the mozbase suite finishes in 51s with -j8 as opposed to 2m24s with -j1.
Not only is mozbase faster, but the full |mach python-test| run is ~50s faster on my local machine with -j8.. Which means we should see this speedup in CI as well. This is simply from mozbase being able to run in parallel.

Before:
./mach python-test -j8  128.43s user 19.96s system 133% cpu 1:50.81 total

After:
./mach python-test -j8  134.79s user 24.11s system 248% cpu 1:03.95 total
Comment on attachment 8812219 [details]
Bug 1317970 - Move mozsystemmonitor tests to a consistent directory structure,

https://reviewboard.mozilla.org/r/94052/#review94624
Attachment #8812219 - Flags: review?(cmanchester) → review+
Comment on attachment 8812220 [details]
Bug 1317970 - Make mozbase tests use mozunit for consistent formatting,

https://reviewboard.mozilla.org/r/94054/#review94628

::: testing/mozbase/mozlog/tests/test_structured.py:848
(Diff revision 1)
> +
> +        # redirect stderr because the output looks like a failure
> +        old_stderr = sys.stderr
> +        sys.stderr = open(os.devnull, 'w')
> +
>          # check that every formatter except raw is not present
>          for fmt in other_formatters:
>              with self.assertRaises(SystemExit):
>                  parser.parse_args(["--log-%s=-" % fmt])
>              with self.assertRaises(SystemExit):
>                  parser.parse_args(["--log-%s-level=error" % fmt])
> +        sys.stderr = old_stderr
> +

This change seems to belong in a different commit, or even a different bug.
Attachment #8812220 - Flags: review?(cmanchester) → review+
Comment on attachment 8812221 [details]
Bug 1317970 - Use manifestparser manifests for python unit tests,

https://reviewboard.mozilla.org/r/94056/#review94638

::: python/mozbuild/mozbuild/backend/recursivemake.py:155
(Diff revision 1)
>      b'MOCHITEST_FILES_PARTS',
>      b'MOCHITEST_METRO_FILES',
>      b'MOCHITEST_ROBOCOP_FILES',
>      b'MODULE_OPTIMIZE_FLAGS',
>      b'MOZ_CHROME_FILE_FORMAT',
> +    b'PYTHON_UNIT_TESTS',

`PYTHON_UNITTEST_MANIFESTS`?
Attachment #8812221 - Flags: review?(cmanchester) → review+
Comment on attachment 8812222 [details]
Bug 1317970 - Filter python unittests through manifestparser.active_tests,

https://reviewboard.mozilla.org/r/94058/#review94642
Attachment #8812222 - Flags: review?(cmanchester) → review+
Comment on attachment 8812221 [details]
Bug 1317970 - Use manifestparser manifests for python unit tests,

https://reviewboard.mozilla.org/r/94056/#review94638

> `PYTHON_UNITTEST_MANIFESTS`?

Oh, actually, this list is just things we can't have in Makefile.in, and it's treated in the same way as `MOZBUILD_VARIABLES`, I think we can just leave this as-is.
Comment on attachment 8812220 [details]
Bug 1317970 - Make mozbase tests use mozunit for consistent formatting,

https://reviewboard.mozilla.org/r/94054/#review94628

> This change seems to belong in a different commit, or even a different bug.

Yeah, true.. I'll file a new bug for it.
Comment on attachment 8812220 [details]
Bug 1317970 - Make mozbase tests use mozunit for consistent formatting,

https://reviewboard.mozilla.org/r/94054/#review94628
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4b8e2baeec2
Move mozsystemmonitor tests to a consistent directory structure, r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/55405892e588
Make mozbase tests use mozunit for consistent formatting, r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/068f2a43fe68
Use manifestparser manifests for python unit tests, r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/9100b758e258
Filter python unittests through manifestparser.active_tests, r=chmanchester
Duplicate of this bug: 1307200
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.