Add mach test package command for running the various harnesses

RESOLVED FIXED in Firefox 50

Status

Testing
General
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
As part of an effort to make debugging tests remotely easy, we are providing an interface to run test harnesses via mach directly from a tests.zip. Running tests from the srcdir would still be preferable, but is likely a long way out. This isn't hard to do and will be useful in the meantime.

This will hook up xpcshell to the mach in the tests.zip.
(Assignee)

Comment 1

2 years ago
Created attachment 8761822 [details]
Bug 1278890 - Search for a firefox binary in the test package mach bootstrap,

Review commit: https://reviewboard.mozilla.org/r/58888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58888/

Updated

2 years ago
Summary: Add mach test package command for running xpcshell → Add mach test package command for running the various harnesses

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 2

2 years ago
Created attachment 8770611 [details]
Bug 1278890 - Add a method for normalizing test paths to the test package mach environment,

The test path structure is slightly different in the test package compared to a srcdir. So we may need
to normalize the specified paths such that they are relative to a srcdir. Because every test harness
needs to do this, this method is being added to the bootstrap for re-use.

Review commit: https://reviewboard.mozilla.org/r/64042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64042/
Attachment #8761822 - Attachment description: Bug 1278890 - Add xpcshell support to mach in the test package → Bug 1278890 - Search for a firefox binary in the test package mach bootstrap,
Attachment #8770611 - Flags: review?(armenzg)
Attachment #8770612 - Flags: review?(armenzg)
Attachment #8770613 - Flags: review?(armenzg)
Attachment #8761822 - Flags: review?(armenzg)
(Assignee)

Comment 3

2 years ago
Created attachment 8770612 [details]
Bug 1278890 - Add xpcshell support to test package mach environment,

This adds the 'xpcshell-test' command to the mach environment found in the test
package. This will allow developers to easily run xpcshell after checking out
and interactive worker.

Review commit: https://reviewboard.mozilla.org/r/64044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64044/
(Assignee)

Comment 4

2 years ago
Created attachment 8770613 [details]
Bug 1278890 - Add reftest support to test package mach environment,

This adds reftest support to the test package mach environment. It will allow
developers to easily run reftests after checking out an interactive worker.

Review commit: https://reviewboard.mozilla.org/r/64046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64046/
(Assignee)

Comment 5

2 years ago
Comment on attachment 8761822 [details]
Bug 1278890 - Search for a firefox binary in the test package mach bootstrap,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58888/diff/1-2/

Comment 6

2 years ago
Comment on attachment 8761822 [details]
Bug 1278890 - Search for a firefox binary in the test package mach bootstrap,

https://reviewboard.mozilla.org/r/58888/#review61026

It looks functionally sound. I just want more comments to help out with giving context.

::: testing/tools/mach_test_package_bootstrap.py:98
(Diff revision 2)
> +        with open(context.mozharness_config, 'r') as f:
> +            config = json.load(f)
> +        workdir = os.path.join(config['base_work_dir'], config['work_dir'])
> +        search_paths.append(os.path.join(workdir, 'application'))
> +
> +    # Check for test-stage setup

What does test-stage mean in this context?

::: testing/tools/mach_test_package_bootstrap.py:99
(Diff revision 2)
> +            config = json.load(f)
> +        workdir = os.path.join(config['base_work_dir'], config['work_dir'])
> +        search_paths.append(os.path.join(workdir, 'application'))
> +
> +    # Check for test-stage setup
> +    dist_bin = os.path.join(os.path.dirname(context.package_root), 'bin')

Isn't this context.bin_dir?

::: testing/tools/mach_test_package_bootstrap.py:110
(Diff revision 2)
> +            return mozinstall.get_binary(path, 'firefox')
> +        except mozinstall.InvalidBinary:
> +            continue
> +
> +
>  def bootstrap(test_package_root):

Could you please add a docstring to this function?
It is becoming rather big.

::: testing/tools/mach_test_package_bootstrap.py:128
(Diff revision 2)
>  
>      def populate_context(context, key=None):
>          if key is not None:
>              return
>  
>          context.package_root = test_package_root

What is package_root?
Attachment #8761822 - Flags: review?(armenzg) → review+

Comment 7

2 years ago
Comment on attachment 8770611 [details]
Bug 1278890 - Add a method for normalizing test paths to the test package mach environment,

https://reviewboard.mozilla.org/r/64042/#review61098

::: testing/tools/mach_test_package_bootstrap.py:112
(Diff revision 1)
>              continue
>  
>  
> +def normalize_test_path(test_root, path):
> +    if os.path.isabs(path) or os.path.exists(path):
> +        return os.path.normpath(os.path.abspath(path))

Do we need to check for isabs() if we're going to call abspath() in the return statement?

Is the path useful if it is absolute, however, it does not exist?
Attachment #8770611 - Flags: review?(armenzg) → review+

Comment 8

2 years ago
Comment on attachment 8770612 [details]
Bug 1278890 - Add xpcshell support to test package mach environment,

https://reviewboard.mozilla.org/r/64044/#review61100
Attachment #8770612 - Flags: review?(armenzg) → review+

Comment 9

2 years ago
Comment on attachment 8770613 [details]
Bug 1278890 - Add reftest support to test package mach environment,

https://reviewboard.mozilla.org/r/64046/#review61102
Attachment #8770613 - Flags: review?(armenzg) → review+
(Assignee)

Comment 10

2 years ago
https://reviewboard.mozilla.org/r/64042/#review61098

> Do we need to check for isabs() if we're going to call abspath() in the return statement?
> 
> Is the path useful if it is absolute, however, it does not exist?

Yeah, we need to check isabs to prevent absolute paths from reaching the code path below (where we try to os.path.join it). You're right to notice that the abspath() call is redundant in that case, but the code is a bit cleaner this way.

Also, absolute paths that don't exist aren't useful, but I return it anyway so the underlying harnesses can handle them however they're supposed to be handled (e.g log an error etc). I don't really want to log these errors at this point as that would make running tests on an interactive worker produce different output from the normal harness.

Comment 11

2 years ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06ca3f2b883b
Search for a firefox binary in the test package mach bootstrap, r=armenzg
https://hg.mozilla.org/integration/autoland/rev/45d6a990dd28
Add a method for normalizing test paths to the test package mach environment, r=armenzg
https://hg.mozilla.org/integration/autoland/rev/3e6f9e3a99a1
Add xpcshell support to test package mach environment, r=armenzg
https://hg.mozilla.org/integration/autoland/rev/683bf5d3c1cd
Add reftest support to test package mach environment, r=armenzg

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06ca3f2b883b
https://hg.mozilla.org/mozilla-central/rev/45d6a990dd28
https://hg.mozilla.org/mozilla-central/rev/3e6f9e3a99a1
https://hg.mozilla.org/mozilla-central/rev/683bf5d3c1cd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.