Closed Bug 984930 Opened 10 years ago Closed 10 years ago

Create mochitest-dt and move all mochitests under browser/devtools into that suite

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: miker, Assigned: jmaher)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: ateam-unittests-big)

Attachments

(15 files, 9 obsolete files)

2.32 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
155.39 KB, text/plain
jmaher
: feedback+
Details
2.68 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
11.31 KB, patch
mozilla
: review+
Details | Diff | Splinter Review
1.56 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
2.74 KB, patch
ahal
: review+
Details | Diff | Splinter Review
9.85 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
24.77 KB, patch
miker
: review+
Details | Diff | Splinter Review
3.82 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
696 bytes, patch
armenzg
: review+
Details | Diff | Splinter Review
4.80 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
1.05 KB, patch
miker
: review+
Details | Diff | Splinter Review
764 bytes, patch
bhearsum
: review+
Details | Diff | Splinter Review
899 bytes, patch
pmoore
: review+
Details | Diff | Splinter Review
16.75 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
Due to us hitting the global timeout in mochitest-bc this is the perfect time for us to move our devtools mochitests into a new test suite.

Can we create a new test suite mochitest-dt and move all mochitests from browser/devtools from mochitest-bc to mochitest-dt?
This is going to require two bits of work:
1) Define the suite in some way. Are we going to use separate manifests for the tests? Are we simply going to have the test harness filter out all the tests under browser/devtools for a normal browser-chrome run? However you choose to do this, you'll need to add some way to invoke this separate "mochitest-dt" suite in the Mochitest harness:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py
(you can look for "ipcplugins" for something similar we do already)
You'll also want to add a mach command to run this test suite:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#580

2) Once you have that working, get the mozharness+buildbot configuration changes necessary to enable a new suite. That's all in Release Engineering's domain, you might want to file a bug on them right now to get that process started.
can we agree on the ideal method for splitting these out via the harness/manifests?
Rather than splitting out devtools and waiting till something else makes bc last too long should we just split bc into slices like plain mochitest already does?
That's bug 819963. Turns out we have a lot of test interdependencies. jmaher has been working on those. philor has expressed interest in moving the devtools tests into their own chunk for a while, since they apparently have a bad track record of being flaky.
:mossop, I have looked into this and I would need to disable 70 tests to make that happen more reliably.  The majority of the tests are in devtools :(

I haven't done the work because I need to file 70 bugs and deal with all the coordination required to either fix the bug (get more logs, etc.) which requires a lot of randomization and waiting vs just disabling everything.  Most developers are more apt to attempt to fix it first prior to a test being disabled.

If there is support for mass disabling and then filing the bugs after the fact, I could prioritize my work on that and make browser-chrome support chunks in the next week or two!
The technical debt in the reliability of browser chrome tests seems to been spilling over into automation more frequently lately. I think these issues should be raised to firefox-dev@mozilla.org and possibly raised at the next Firefox team meeting and with Firefox team management. CC'ing a few managers.
(In reply to Gregory Szorc [:gps] from comment #6)
> The technical debt in the reliability of browser chrome tests seems to been
> spilling over into automation more frequently lately. I think these issues
> should be raised to firefox-dev@mozilla.org and possibly raised at the next
> Firefox team meeting and with Firefox team management. CC'ing a few managers.

Please do, I've not heard anything about this before.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> This is going to require two bits of work:
> 1) Define the suite in some way. Are we going to use separate manifests for
> the tests? Are we simply going to have the test harness filter out all the
> tests under browser/devtools for a normal browser-chrome run?

I think it is best if the harness filters out all browser-chrome tests under browser/devtools for a normal browser-chrome run?

> However you
> choose to do this, you'll need to add some way to invoke this separate
> "mochitest-dt" suite in the Mochitest harness:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py
> (you can look for "ipcplugins" for something similar we do already)

Yup, planning on it.

> You'll also want to add a mach command to run this test suite:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/
> mach_commands.py#580
> 

You mean as opposed to ./mach mochitest-browser browser/devtools ?

We could use ./mach devtools-test but these are still browser-chrome tests.

> 2) Once you have that working, get the mozharness+buildbot configuration
> changes necessary to enable a new suite. That's all in Release Engineering's
> domain, you might want to file a bug on them right now to get that process
> started.

Bug 985350 logged.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> > You'll also want to add a mach command to run this test suite:
> > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/
> > mach_commands.py#580
> > 
> 
> You mean as opposed to ./mach mochitest-browser browser/devtools ?
> 
> We could use ./mach devtools-test but these are still browser-chrome tests.

If you're naming the test something different on tinderbox, we need at least an alias to run it via mach. We have aliases in the "mach test" command, but I don't think they support an alias that evaluates to a subdirectory currently:
http://mxr.mozilla.org/mozilla-central/source/testing/mach_commands.py#44

FWIW, jmaher and I were discussing a possible generic way to implement this yesterday, by adding some sort of "subsuite" key to the manifest files, like:

[DEFAULT]
subsuite = devtools

and then requiring an invocation like "mach mochitest-browser --subsuite=devtools" (which you could obviously alias to "mach mochitest-devtools" or something).
the work for this would include:
* annotate manifests with subsuite keyword (just add it to the default section of all browser.ini in devtools folder)
* add cli option for mochitest for --subsuite
* modify runtests.py mochitest harness to ignore all tests that have a subsuite defined unless one is passed in, then match on it (note: leave the flag to ignore subsuite stuff until the last push)
* modify the mach harness to support browser-dt, probably add in 'mach developer-tools' as an option
* modify buildbot to add a new test type with the additional commandline args: '--subsuite=devtools'
* modify trychooser to have the new M(dt) option to choose from
* modify tbpl/treeherder to support M(dt)

I see that as being mostly in order of operations.
Just for kicks, here's a Try run with all devtools tests disabled to see what the affect on mochitest-bc would be. And as an aside, the moz.build files inside browser/devtools are a bit of rats nest. Would be nice if we could clean those up independent of the rest of this.

https://tbpl.mozilla.org/?tree=Try&rev=a7fe587af6f7
Moral of the story from the Try run:
* Runtimes on opt builds dropped by 50%, roughly in line with the reduction in number of tests run.
* Debug build runtimes dropped by closer to 60% (linux64 for example was 140+ -> ~60, linux32 170+ -> ~70)
* It wasn't a cure-all for our orange woes. browser_newtab_bug735987.js (bug 898317) continues to hurt Windows badly. Sessionstore tests continue to have trouble. And Linux debug is still bad. Really bad. "Why is this even visible?" bad.

I think this shows that mochitest-bc would benefit tremendously from mochitest-dt. We can also judge independently at that point whether it is a suite that meets TBPL visibility standards without impacting everything else that runs in mochitest-bc.
Attached patch 1 Add mach mochitest-devtools (obsolete) — Splinter Review
(In reply to Joel Maher (:jmaher) from comment #10)
> the work for this would include:
> * annotate manifests with subsuite keyword (just add it to the default
> section of all browser.ini in devtools folder)

Done

> * add cli option for mochitest for --subsuite

Done

> * modify runtests.py mochitest harness to ignore all tests that have a
> subsuite defined unless one is passed in, then match on it (note: leave the
> flag to ignore subsuite stuff until the last push)

Done

> * modify the mach harness to support browser-dt, probably add in 'mach
> developer-tools' as an option

Done

The following still remain as I don't believe I have access to the necessary repos so somebody else will need to take these on:
> * modify buildbot to add a new test type with the additional commandline
> args: '--subsuite=devtools'
> * modify trychooser to have the new M(dt) option to choose from
> * modify tbpl/treeherder to support M(dt)
Attachment #8396301 - Flags: review?(jmaher)
Comment on attachment 8396301 [details] [diff] [review]
1 Add mach mochitest-devtools

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

Thanks for putting this together!

while I have no specific plan for other subsuites, this has a lot of value for other uses.  I am fine with a followup bug to fully support subsuite and make the changes outlined below.

Keep in mind that when we land this all devtools will stop working until the mozharness bits and buildbot changes are made.  We will need to land this on b2g-inbound, m-c, inbound, fx-team and any other trunk based tree.  Either land on all of them together or just let it get merged and live without coverage for ~6 hours.  We would just need to coordinate with the Sheriff's and releng on getting this out and then doing a buildbot config right afterwards.

Another option is to require another command-line option that doesn't run subsuites (--no-subsuites).  This would make it easier to coordinate with releng on deploying buildbot and mozharness changes.

r+ assuming we file a followup bug for manifestparser/runtests to support arbitrary subsuites and addressing the two nits below.

::: testing/mochitest/runtests.py
@@ +308,5 @@
>      # allow relative paths for logFile
>      if options.logFile:
>        options.logFile = self.getLogFilePath(options.logFile)
> +    if options.browserChrome or options.chrome or options.subsuite or \
> +       options.a11y or options.webapprtChrome:

this assumes subsuite is a chrome/browser-chrome style test.  If we made webgl a subsuite (which is run in mochitest-plain) this would fail.  I don't have a specific recommendation for this now other than to add a comment and file a followup bug.

::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
@@ +1096,5 @@
> +        if options:
> +            if options.devtools:
> +                tests = [test for test in tests if test['subsuite'] == 'devtools']
> +            else:
> +                tests = [test for test in tests if test['subsuite'] != 'devtools']

again, this is hardcoded for this suite, I think a future proofing way would be to set tests initially like this:
tests = [test for test in tests if not test['subsuite']]

then:
if options and options.devtools:
 tests = [test for test in tests if test['subsuite'] == 'devtools']
Attachment #8396301 - Flags: review?(jmaher) → review+
Armen, how much work is it to create a mochitest-dt run from buildbot?  I used to be familiar with the whole process, but since mozharness was rolled out I haven't fiddled much with it.  Could you outline the high level steps required from the mozharness/buildbot perspective to do this?
Flags: needinfo?(armenzg)
Here are the different pieces involved.

1) Add it to MOCHITEST [1]
** That will add it across the board for all platforms for all branches
** If we need less branches or less platforms we can modify it with something like this [2]
2) Add it to mozharness [3][4][5]
3) Add it to trychooser [6]

Step #1 can vary wildly depending if a different configuration is needed. Enabling everywhere requires everything to be "green". I assume that will require some branch limitations until then [2].

[1] http://hg.mozilla.org/build/buildbot-configs/file/default/mozilla-tests/config.py#l376
[2] http://hg.mozilla.org/build/buildbot-configs/file/default/mozilla-tests/config.py#l1825
[3] http://hg.mozilla.org/build/mozharness/file/f0bb54c56271/configs/unittests/linux_unittest.py#l53
[4] http://hg.mozilla.org/build/mozharness/file/f0bb54c56271/configs/unittests/mac_unittest.py#l44
[5] http://hg.mozilla.org/build/mozharness/file/f0bb54c56271/configs/unittests/win_unittest.py#l53
[6] http://hg.mozilla.org/build/tools/file/4e132591752e/trychooser
Flags: needinfo?(armenzg)
Attached patch 1 Add mach mochitest-devtools (obsolete) — Splinter Review
(In reply to Joel Maher (:jmaher) from comment #14)
> Comment on attachment 8396301 [details] [diff] [review]
> 1 Add mach mochitest-devtools
> 
> Review of attachment 8396301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for putting this together!
> 
> while I have no specific plan for other subsuites, this has a lot of value
> for other uses.  I am fine with a followup bug to fully support subsuite and
> make the changes outlined below.
> 
> Keep in mind that when we land this all devtools will stop working until the
> mozharness bits and buildbot changes are made.  We will need to land this on
> b2g-inbound, m-c, inbound, fx-team and any other trunk based tree.  Either
> land on all of them together or just let it get merged and live without
> coverage for ~6 hours.  We would just need to coordinate with the Sheriff's
> and releng on getting this out and then doing a buildbot config right
> afterwards.
> 
> Another option is to require another command-line option that doesn't run
> subsuites (--no-subsuites).  This would make it easier to coordinate with
> releng on deploying buildbot and mozharness changes.
> 
> r+ assuming we file a followup bug for manifestparser/runtests to support
> arbitrary subsuites and addressing the two nits below.
> 
> ::: testing/mochitest/runtests.py
> @@ +308,5 @@
> >      # allow relative paths for logFile
> >      if options.logFile:
> >        options.logFile = self.getLogFilePath(options.logFile)
> > +    if options.browserChrome or options.chrome or options.subsuite or \
> > +       options.a11y or options.webapprtChrome:
> 
> this assumes subsuite is a chrome/browser-chrome style test.  If we made
> webgl a subsuite (which is run in mochitest-plain) this would fail.  I don't
> have a specific recommendation for this now other than to add a comment and
> file a followup bug.
> 

Added comment, bug 987743 filed.

> ::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
> @@ +1096,5 @@
> > +        if options:
> > +            if options.devtools:
> > +                tests = [test for test in tests if test['subsuite'] == 'devtools']
> > +            else:
> > +                tests = [test for test in tests if test['subsuite'] != 'devtools']
> 
> again, this is hardcoded for this suite, I think a future proofing way would
> be to set tests initially like this:
> tests = [test for test in tests if not test['subsuite']]
> 
> then:
> if options and options.devtools:
>  tests = [test for test in tests if test['subsuite'] == 'devtools']

Or even better:
Set options.subsuite to 'devtools' for devtools stuff

if options and options.subsuite:
    tests = [test for test in tests if options.subsuite == test['subsuite']]
else:
    tests = [test for test in tests if not test['subsuite']]

This way nobody needs to touch this section of code again.

Now adding a subsuite is simply a case of:
1. Add subsuite = devtools to [DEFAULT] section of browser.ini file in each subsuite folder.

2. Add subsuite in testing/mach_commands.py
+    'mochitest-devtools': {
+        'aliases': ('dt', 'DT', 'Dt'),
+        'mach_command': 'mochitest-browser --subsuite=devtools',
+        'kwargs': {'test_file': None},
+    },

3. Act on suite in testing/mochitest/mach_commands.py:
+        elif suite == 'devtools':
+            options.browserChrome = True
+            options.subsuite = 'devtools'
...
+    @Command('mochitest-devtools', category='testing',
+        conditions=[conditions.is_firefox],
+        description='Run a devtools mochitest with browser chrome.')
+    @MochitestCommand
+    def run_mochitest_devtools(self, test_file, **kwargs):
+        return self.run_mochitest(test_file, 'devtools', **kwargs)

4. Add the suite to testing/testsuite-targets.mk:
-MOCHITESTS := mochitest-plain mochitest-chrome mochitest-a11y mochitest-ipcplugins
+MOCHITESTS := mochitest-plain mochitest-chrome mochitest-devtools mochitest-a11y mochitest-ipcplugins
...
+mochitest-devtools:
+	$(RUN_MOCHITEST) --subsuite=devtools
+	$(CHECK_TEST_ERROR)
...
.PHONY: \
   mochitest \
   mochitest-plain \
   mochitest-chrome \
+  mochitest-devtools \
   mochitest-a11y \
Attachment #8396433 - Flags: review+
Attachment #8396433 - Attachment description: 1 → 1 Add mach mochitest-devtools
Attachment #8396301 - Attachment is obsolete: true
Attached patch 1 Add mach mochitest-devtools (obsolete) — Splinter Review
subsuite = devtools was somehow dropped from markup view... added it again.
Attachment #8396433 - Attachment is obsolete: true
Attachment #8396518 - Flags: review+
Attachment #8397185 - Flags: review?(armenzg)
Depends on: 988413
Talking to jmaher on IRC, I think we should call this job mochitest-devtools-chrome to better match the convention of other tests running in this harness (like mochitest-metro-chrome). That's the job name I'm going to base the TBPL patch off.
adjusting internal name to mochitest-devtools-chrome
Attachment #8397185 - Attachment is obsolete: true
Attachment #8397185 - Flags: review?(armenzg)
Attachment #8397216 - Flags: review?(armenzg)
adjusting internal name to mochitest-devtools-chrome
Attachment #8397186 - Attachment is obsolete: true
Attachment #8397186 - Flags: review?(armenzg)
Attachment #8397217 - Flags: review?(armenzg)
Attachment #8397216 - Flags: review?(armenzg) → review+
Comment on attachment 8397217 [details] [diff] [review]
buildbot custom changes to support mochitest-devtools (1.1)

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

I will attach the list of builders that get added with this patch.

In my opinion we should see this running first on one or two branches (e.g. mozilla-inbound, try) and then when ready enable across the board.

Otherwise, if it is not green we would have to visit every tbpl page to hide the builder.

::: mozilla-tests/config.py
@@ +414,4 @@
>          'script_path': 'scripts/desktop_unittest.py',
>          'extra_args': ['--mochitest-suite', 'browser-chrome'],
>          'blob_upload': True,
> +        'script_maxtime': 7200,

As long as the jobs are finishing these days withing that range.

@@ +455,4 @@
>          'script_path': 'scripts/desktop_unittest.py',
>          'extra_args': ['--mochitest-suite', 'browser-chrome-3'],
>          'blob_upload': True,
> +        'script_maxtime': 4200,

I assume not intentional.

@@ +1698,3 @@
>          'branches': ['fx-team'],
>          'platforms': {
>              'ubuntu64_vm': {'ext': 'linux-x86_64.tar.bz2', 'debug': True},

I assume not intentional.
Attachment #8397217 - Flags: review?(armenzg) → review+
Attached file builders that change
Attachment #8397242 - Flags: feedback?(jmaher)
Comment on attachment 8397242 [details]
builders that change

that is a lot of branches, but the changes look right (platforms + pgo/opt/debug)
Attachment #8397242 - Flags: feedback?(jmaher) → feedback+
updated to support cedar only for a trial run, r+ via irc
Attachment #8397217 - Attachment is obsolete: true
Attachment #8397269 - Flags: review+
Comment on attachment 8397216 [details] [diff] [review]
[checked-in] mozharness changes to support devtools (1.1)

this patch is Live in production
Comment on attachment 8396518 [details] [diff] [review]
1 Add mach mochitest-devtools

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

::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
@@ +1095,5 @@
> +        # Filter on current subsuite
> +        if options and options.subsuite:
> +            tests = [test for test in tests if options.subsuite == test['subsuite']]
> +        else:
> +            tests = [test for test in tests if not test['subsuite']]

previously this had been:
if options:
    if options.subsuite:
        tests = <matching subsuite>
    else:
        tests = <not matching subsuite>

I tested locally before pushing to cedar, and switching back to this logic works.  The reason why is the build system was filtering the non subsuite tests out and they didn't exist- so since the build system doesn't pass in options, we work fine with the extra if nesting.
I have pushed this up to cedar and most likely tomorrow we will have the buildbot-config changes for there.  We can see this live and next week we can roll out to all integration and project branches.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> Talking to jmaher on IRC, I think we should call this job
> mochitest-devtools-chrome to better match the convention of other tests
> running in this harness (like mochitest-metro-chrome). That's the job name
> I'm going to base the TBPL patch off.

These are browser mochitests, but we also have devtools tests of the mochitest-chrome variant. Oh well.
Yeah, it's a bit unfortunate how things are named, but realistically, it's just a behind the scenes name anyway.
in production
Attachment #8397982 - Flags: review?(aki)
Comment on attachment 8397982 [details] [diff] [review]
add which configs to use for each platform

We have to rename.
Attachment #8397982 - Flags: review?(aki)
Attachment #8397982 - Attachment is obsolete: true
Attachment #8397990 - Flags: review?(aki)
Attachment #8397990 - Flags: review?(aki) → review+
Live in production.
Depends on: 960481
Depends on: 989168
if we fix /browser/devtools/app-manager/test/browser_manifest_editor.js for linux opt (bug 989168), that would clean up about 1/3 of the failures, and almost all the rest are in styleinspector tests.  Personally we could probably fix one or two style inspector tests and call this 95% green.
Attachment #8396518 - Attachment is obsolete: true
Attachment #8398472 - Flags: review?(mratcliffe)
this is not mission critical, but this should get us support for the remaining platforms.
Attachment #8398524 - Flags: review?(armenzg)
Attachment #8398524 - Flags: review?(armenzg) → review+
asan + 10.9 is landed:
https://hg.mozilla.org/build/buildbot-configs/rev/fc2f1e3b94f3

will be picked up in the next reconfig.
Attachment #8398472 - Flags: review?(mratcliffe) → review+
Comment on attachment 8398472 [details] [diff] [review]
update for bitrot and added fixes for tests (3.0)

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

The "mach mochitest-devtools" part of this patch doesn't seem to work for me.

I get the following error:

TypeError: run_mochitest_devtools() takes exactly 2 arguments (1 given)
     
 File "/Users/jryans/projects/mozilla/gecko-dev-2/python/mach/mach/main.py", line 416, in _run
   result = fn(**vars(args.command_args))
jmaher sent me a small diff that fixes this error, and I can now run the new suite locally.

However, the new "mach mochitest-devtools" command appears to not be able to run just a single file or directory (instead it requires the full suite to be run) at the moment, so I think for now I will continue debugging without it.  We should definitely add this functionality now / very soon!
in production.
Attachment #8400685 - Flags: review?(ahalberstadt) → review+
I will look at the patch tomorrow. Please poke in #releng for a reviewer if you would prefer to speed this up.
mozharness changes landed on default:
https://hg.mozilla.org/build/mozharness/rev/19d4c2bb576e

we need these to land prior to the buildbot changes, so we can wait until tomorrow for the buildbot-config changes.
Depends on: 991267
Depends on: 980746
Comment on attachment 8397216 [details] [diff] [review]
[checked-in] mozharness changes to support devtools (1.1)

https://hg.mozilla.org/build/mozharness/rev/7581de0051fa

Do we need definitions for both m-d-c and chunked m-d-c?
Attachment #8397216 - Attachment description: mozharness changes to support devtools (1.1) → [checked-in] mozharness changes to support devtools (1.1)
(In reply to Armen Zambrano [:armenzg] (Release Engineering) (EDT/UTC-4) from comment #52)
> Comment on attachment 8397216 [details] [diff] [review]
> [checked-in] mozharness changes to support devtools (1.1)
> 
> https://hg.mozilla.org/build/mozharness/rev/7581de0051fa
> 
> Do we need definitions for both m-d-c and chunked m-d-c?

NVM. I see what's going on.
Attachment #8400688 - Flags: review?(armenzg) → review+
Attachment #8397269 - Attachment description: buildbot configs for devtools - cedar only (2.0) → [checked-in] buildbot configs for devtools - cedar only (2.0)
Attachment #8397990 - Attachment description: add which configs to use for each platform → [checked-in] add which configs to use for each platform
Attachment #8398524 - Attachment description: support for linux64-asan and osx 10.9 (1.0) → [checked-in] support for linux64-asan and osx 10.9 (1.0)
Attachment #8400685 - Attachment description: mozharness changes for 2 chunk devtools (1.0) → [checked-in] mozharness changes for 2 chunk devtools (1.0)
buildbot configs for 2 chunk debug devtools landed:
https://hg.mozilla.org/build/buildbot-configs/rev/b20a0773d06c
Depends on: 992270
Depends on: 986452
Blocks: 992485
Depends on: 992611
Depends on: 964369
Now tests run fine when a subpath is included.
Attachment #8398472 - Attachment is obsolete: true
Attachment #8402605 - Flags: review?(jmaher)
Comment on attachment 8402605 [details] [diff] [review]
Switched back to test_paths and fixed running tests under a path.

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

this looks good.  Glad you figured out the test path stuff.  We have a few more subsuites to add:
browser/devtools/canvasdebugger
browser/devtools/webaudioeditor
toolkit/devtools/server/test/browser

I would really like to get this landed, but turn off the default filtering.  On cedar we are close, but have 5 or 6 bugs in the works (i.e. actively testing fixes).  This way we can avoid bitrot and keep any future patches much smaller.  Once we are ready to make the switch (a week or so), we can turn on the filtering by default.  I believe that will be just a small change to line 469 in runtests.py.

::: python/mozbuild/mozbuild/testing.py
@@ +91,5 @@
>          def fltr(tests):
>              for test in tests:
> +                if flavor and \
> +                   (flavor == 'devtools' and test.get('flavor') != 'browser-chrome') or \
> +                   flavor != 'devtools' and test.get('flavor') != flavor:

I am not an expert at python left to right parsing of expressions, but I would personally add a set of ()'s around the last line here to be more clear.
Attachment #8402605 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #58)
> Comment on attachment 8402605 [details] [diff] [review]
> Switched back to test_paths and fixed running tests under a path.
> 
> Review of attachment 8402605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks good.  Glad you figured out the test path stuff.  We have a few
> more subsuites to add:
> browser/devtools/canvasdebugger
> browser/devtools/webaudioeditor
> toolkit/devtools/server/test/browser
> 

Added.

> I would really like to get this landed, but turn off the default filtering. 
> On cedar we are close, but have 5 or 6 bugs in the works (i.e. actively
> testing fixes).  This way we can avoid bitrot and keep any future patches
> much smaller.  Once we are ready to make the switch (a week or so), we can
> turn on the filtering by default.  I believe that will be just a small
> change to line 469 in runtests.py.
> 

To be honest I am not sure which filtering you are talking about.

> ::: python/mozbuild/mozbuild/testing.py
> @@ +91,5 @@
> >          def fltr(tests):
> >              for test in tests:
> > +                if flavor and \
> > +                   (flavor == 'devtools' and test.get('flavor') != 'browser-chrome') or \
> > +                   flavor != 'devtools' and test.get('flavor') != flavor:
> 
> I am not an expert at python left to right parsing of expressions, but I
> would personally add a set of ()'s around the last line here to be more
> clear.

Agreed, done.
Attachment #8402605 - Attachment is obsolete: true
Attachment #8402634 - Flags: review+
I was thinking a way to turn this off would be to not add the options into active_tests in runtests.py:
 tests = manifest.active_tests(disabled=True, options=options, **info)

Since we wouldn't be passing in options, then we wouldn't filter.  This leaves us with a one line change to alter everything.  I am open to other thoughts.

Thanks for updating the patch to include everything.  On Cedar we have a few fixes to tests landed, a dozen tests disabled on there, and a few harness tweaks.  With all of that we are just about as green as a leprechaun- quite possibly this week we can make all of it official and landed so we can make this change officially.  I view landing the bulk of these changes as part of the prep work, one less mystery.
Attachment #8402806 - Flags: review?(armenzg) → review+
Attachment #8402807 - Flags: review?(armenzg) → review+
Attachment #8403406 - Flags: review?(armenzg) → review+
landed followup for 3rd dt chunk on buildbot-configs:
https://hg.mozilla.org/build/buildbot-configs/rev/c0ddd294a88e
Depends on: 993580
this is a temporary patch to be applied on top of the mochitest harness patch.  We are looking to enable this tomorrow if all goes well.  In order for that to happen, we need to have the mochitest test harness support --subsuite.
Attachment #8404705 - Flags: review?(mratcliffe)
Attachment #8404705 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/df3c1971ecdc
Assignee: nobody → jmaher
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Believe this was meant to be leave-open?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Joel Maher (:jmaher) from comment #69)
> landed:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/52da0fc935ce

Landed with the wrong bug number (ended up in bug 983948); was merged at the same time as comment 70:
https://hg.mozilla.org/mozilla-central/rev/52da0fc935ce
Attachment #8405334 - Flags: review?(pmoore)
Attachment #8405334 - Flags: review?(pmoore) → review+
Blocks: 995186
Ryan and Joel reviewed this over IRC over the course of many hours. I've checked it in, and I'm about to start a reconfig.
Attachment #8405463 - Flags: review+
Depends on: 995529
Comment on attachment 8405388 [details] [diff] [review]
only add mavericks tests if we are on cedar (1.0)

Seems a reasonable change to me, but as discussed on IRC, we probably want to go with Ben's larger refactoring.
Attachment #8405388 - Flags: review?(pmoore) → review+
Since this landed: Bug 984930 - "mochitest-devtools mach" command doesn't allow to run single folders or files
Depends on: 995972
(In reply to Joe Walker [:jwalker] from comment #78)
> Since this landed: Bug 984930 - "mochitest-devtools mach" command doesn't
> allow to run single folders or files

The fix is in bug 995972
I've updated the documentation at https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests to point to mochitest-devtools instead of mochitest-browser.  These were the only docs I could find referencing the test suites for devtools.
Bug 995972 still needs to land to fix running single directories with mach, but AFAIK, no other follow-up work is pending at this point. Bug 996240 is filed to track getting this enabled on Aurora30 as well.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 996634
Depends on: 998788
You need to log in before you can comment on or make changes to this bug.