Closed Bug 1305242 Opened 4 years ago Closed 4 years ago

Disable linux64-jsdcov & linux64-ccov from running with '-p all' & '-u all' together.

Categories

(Testing :: Code Coverage, defect)

defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: sparky, Assigned: sparky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We'll have to disable linux64-jsdcov and linux64-ccov from running with a try message of "try: -b o -p all -u all -t none". It only starts if we have '-u all' with '-p all'.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch bug1305242.patch (obsolete) — Splinter Review
Dustin, is this behavior expected? i.e. if run-on-projects is set to [] for both the test and the build, should they be able to run if the options "-p all" and "-u all" are used in the try message? It's what seems to be causing linux64-ccov and linux64-jsdcov from running. We find that this is an urgent issue so I've attached a patch to fix this temporarily since the builds are running alongside everything else and it could get confusing for people since these platforms and their tests are not meaningful to their code. If it's fine for the moment, I'll push it to review.

I am thinking of adding a different condition that would check if "-u all" and "-p all" is being used in the try message and then determine if the build and test definition have run-on-projects defined as an empty sets and prevent them from being scheduled. I believe that this could be implemented relatively easily. Does that sound reasonable? If not, would you have any ideas for how this could be solved?

Thanks!
Flags: needinfo?(dustin)
Luckily, it was a very simple change to make this not platform specific. Let me know what you think. I've checked the target-tasks and it only removes linux64-ccov, and linux64-jsdcov tests from the tasks. Here's a link to a diff viewer: https://www.diffchecker.com/Ecwjx13p . On the left is after the patch and on the right is before the patch.
Attachment #8794619 - Attachment is obsolete: true
Assignee: nobody → gmierz2
Flags: needinfo?(dustin)
Comment on attachment 8794638 [details] [diff] [review]
bug1305242.patch (2)

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

You've found the issue: try_option_syntax wasn't updated to pay attention to run_on_projects for test tasks in bug 1278402, so it was selecting the test tasks based on the coverage builds regardless of that attribute.

The solution, however, is still a little more purpose-specific than it needs to be.  The match_test function has

             if try_spec is not None:
                ...
             return True

and it's that last `return True` that's causing these tasks to be scheduled.  So that is probably the right place look at run_on_projects.

It's probably a good idea to factor out the check for try or all being in run_on_projects into a utility function similar to `match_test`.  You might also want to reverse the conditional (so, `if try_spec is None: ..`) to make the code a little more readable.
Attachment #8794638 - Flags: review-
Sounds good, I'll try to have something done for this soon. Thanks Dustin!
(In reply to Greg Mierzwinski from comment #5)
>

Disregard this attempt. Here's a link to one that works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=645dd3e40a800654b3fd35f5911e89babcd088cf

Here's a link to a diff for '-p all -u all' generated with 'mach taskgraph target':
https://www.diffchecker.com/VPvTr0wd
Comment on attachment 8795589 [details]
Bug 1305242 - Disable linux64-jsdcov and linux64-ccov from running on try with '-u all'.

https://reviewboard.mozilla.org/r/81546/#review80422

::: taskcluster/taskgraph/try_option_syntax.py:492
(Diff revision 1)
> +        def check_run_on_projects():
> +            return set(['try', 'all']) & set(attr('run_on_projects', []))

nice!

::: taskcluster/taskgraph/try_option_syntax.py:498
(Diff revision 1)
>              if self.platforms is not None:
>                  if attr('build_platform') not in self.platforms:
>                      return False
> -            if try_spec is not None:
> +            else:
> +                if not check_run_on_projects():
> +                    return False
> +            if try_spec is None:
> +                return True

So the net effect, for a linux64 gtest, say, is

	cmdline                       r_o_p  !r_o_p
	-------                       -----  ------
	-p all -u all                 yes    no
	-p linux64 -u all             yes    yes *
	-p all -u gtest               yes    no  *
	-p linux64 -u gtest           yes    yes
 
where r_o_p means the gtest's run_on_projects matches `try`, and !r_o_p is the
opposite.

I think the cells marked with * are not what's intended.  From the perspective
of someone thinking about tests, I would expect setting `run_on_projects: []`
to mean "don't run this test on try with `-u all`."  But for the jsdcov/ccov
case you're working on, it's "don't run this test on try with `-p all`."

The "correct" perspective becomes a bit clearer when looking at the task
description:

	mochitest-browser-chrome:
		# ...
    	run-on-projects:
        	by-test-platform:
            	linux64-jsdcov/opt: []
            	linux64-ccov/opt: []
            	default: ['all']

So we are already accounting for the `-p` platform here.  Reiterating the
analysis above, with "runs?" indicating whether the ccov mochitest-bc task will
run:

	cmdline                       		runs?
	-------                       		-----
	-p all -u all                 		no
	-p linux64 -u all             		no
	-p linux64-ccov -u all				yes
	-p all -u mochitest-bc              no  +
	-p linux64 -u mochitest-bc          yes *
	-p linux64-ccov -u mochitest-bc		yes

Here the * is again unexpected.  The + should probably be "no", but maybe we could make an allowance for that being "yes" here, just to keep the logic simple?

So unless I've missed something here, I think you will want to change that
logic around so that all three starred cases change their meaning.  Please also
update `taskcluster/docs/attributes.rst`.  Proposed updated text:

	For try, this attribute applies only for the ``all`` cases; all jobs can be
	specified by name regardless of ``run_on_projects``.  For build tasks,
	``run_on_projects`` is consulted on ``-p all``; for unittest tasks, on ``-u
	all``; and for talos tasks, on ``-t all``.

If you'd like to keep + set to "no", then the text, and implementation, will
need to be even more complex.

If you're feeling adventurous, validating these cases using a unit test would
be pretty cool!
Andrew, I know you hate me by now for flagging you on all of these try bugs, but this raises an interesting point about what "try by default" means for test jobs, so I thought I'd Cc you as an FYI.
Comment on attachment 8795589 [details]
Bug 1305242 - Disable linux64-jsdcov and linux64-ccov from running on try with '-u all'.

https://reviewboard.mozilla.org/r/81546/#review80456

r- for that comment, but I could also be convinced that this behavior is desirable (or that I've mis-interpreted the behavior)
Attachment #8795589 - Flags: review?(dustin) → review-
Dustin, I've done some analysis and for the runs analysis that you did, I found that it was the following in practice after applying the patch:

	cmdline                       		runs?
	-------                       		-----
	-p all -u mochitest-bc                  no +
	-p linux64 -u mochitest-bc              no *

Here are some links to diffs of target-tasks for these two cases:
-p linux64 -u mochitest-bc:
https://www.diffchecker.com/l7tiaQEC

-p all -u mochitest-bc:
https://www.diffchecker.com/oa8QM8TP

I didn't find any unexpected results from the target-tasks for the gtest analysis also. So unless something locally on my end isn't working then the patch doesn't have those unintended consequences. I say this because the diffs above have some different output as target-tasks from |mach taskgraph decision| in comparison to what I get from |mach taskgraph target|. I think it would be a good idea for me to write up a unit test for this though just to make sure. Otherwise, I could just implement a "runs" flag instead of using "run-on-projects"  for the sake of clarity. It would have to be the more complex option since we don't want it in "-p all" for any case since it could confuse developers with errors that may have nothing to do with their code. I think we may be able to allow at least that though.
Flags: needinfo?(dustin)
Indeed, you're right:

$ sed -i -e 's/^message:.*/message: "try: -b o -p all -u all"/g' parameters.yml && ./mach taskgraph target-graph -p parameters.yml 2>/dev/null  | grep linux64-ccov

$ sed -i -e 's/^message:.*/message: "try: -b o -p linux64 -u all"/g' parameters.yml && ./mach taskgraph target-graph -p parameters.yml 2>/dev/null  | grep linux64-ccov

$ sed -i -e 's/^message:.*/message: "try: -b o -p linux64-ccov -u all"/g' parameters.yml && ./mach taskgraph target-graph -p parameters.yml 2>/dev/null  | grep linux64-ccov
build-linux64-ccov/opt
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-1
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-2
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-3
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-4
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-5
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-6
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-7

$ sed -i -e 's/^message:.*/message: "try: -b o -p all -u mochitest-bc"/g' parameters.yml && ./mach taskgraph target-graph -p parameters.yml 2>/dev/null  | grep linux64-ccov

$ sed -i -e 's/^message:.*/message: "try: -b o -p linux64 -u mochitest-bc"/g' parameters.yml && ./mach taskgraph target-graph -p parameters.yml 2>/dev/null  | grep linux64-ccov

$ sed -i -e 's/^message:.*/message: "try: -b o -p linux64-ccov -u mochitest-bc"/g' parameters.yml && ./mach taskgraph target-graph -p parameters.yml 2>/dev/null  | grep linux64-ccov
build-linux64-ccov/opt
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-1
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-2
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-3
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-4
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-5
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-6
desktop-test-linux64-ccov/opt-mochitest-browser-chrome-7

I spent a long time thinking about comment 8, but apparently was completely wrong about `-p linux64 -u mochitest-bc`.  Sorry about that, and about the time you spent refuting my assertions :(

So the full table is:

	cmdline                       		runs?
	-------                       		-----
	-p all -u all                 		no
	-p linux64 -u all             		no
	-p linux64-ccov -u all			yes
	-p all -u mochitest-bc                  no
	-p linux64 -u mochitest-bc              no
	-p linux64-ccov -u mochitest-bc		yes

which is the behavior you want here.

Basically, for a test in a try push, run-on-projects applies only to `-p all` and not to `-u all`.

So, the change I'd like to see to the docs is to this sentence:
  If run_on_projects is set to an empty list, then the task will not run anywhere, unless specified explicitly in try syntax.
should be
  ..unless its build platform is specified explicitly in try syntax.
Any other tweaks are fine too, as you hint in comment 11.
Flags: needinfo?(dustin)
No worry! :) I'm sorry for the delay, I was also gone for almost a week and have had to catch up on some things once I returned. I won't make the name changes to the flag if everything is running properly since it makes more sense this way based on it's effects and how it prevents it from running when '-p all' is used and still allows '-u all' for the build itself. I'll quickly make those changes to the docs though and push a new patch soon.
Comment on attachment 8795589 [details]
Bug 1305242 - Disable linux64-jsdcov and linux64-ccov from running on try with '-u all'.

https://reviewboard.mozilla.org/r/81546/#review84560

Thanks again for your patience :)
Attachment #8795589 - Flags: review?(dustin) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16e0dc7f14d8
Disable linux64-jsdcov and linux64-ccov from running on try with '-u all'. r=dustin
https://hg.mozilla.org/mozilla-central/rev/16e0dc7f14d8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
So right now if I push a patch to try with -p all -u all then linux64-jsdcov is not running. Then when I send my patch to inbound linux64-jsdcov is again, not running. BUT on mc linux64-jsdcov is running and if it fails my patch is backed out. This is super frustrating. We either care about this test suite and then I should be able to see if they fail before my patch hits mc, or we don't care about them and we should not back out patches because of it. I don't think the current state is ideal.
Flags: needinfo?(gmierz2)
Thanks for pointing this out :gabor. So the reasoning behind doing it this way is to prevent wasting resources because collecting code coverage is quite demanding. That said, I didn't know we we're backing out patches if they failed on linux64-jsdcov. Usually (or based on the last time we had a failure on linux64-jsdcov), a bug is filed, and then we either disable it or fix it on the spot without any backouts.

Joel, would you be able to tell me if patches are being backed out for failures on linux64-jsdcov now? I remember there was some talk not too long ago about getting the tiers changed on the code coverage builds.
Flags: needinfo?(gmierz2) → needinfo?(jmaher)
(In reply to Greg Mierzwinski [:gmierz] from comment #19)
> they failed on linux64-jsdcov. Usually (or based on the last time we had a
> failure on linux64-jsdcov), a bug is filed, and then we either disable it or
> fix it on the spot without any backouts.

Thanks for the quick reply, and yeah, this would make perfect sense. I'm also not sure what this test actually does. Is there a wiki page for it? I wonder if I can run this jsdconv setting on OSX or Windows or it's a linux specific thing, and of course how can I run a test in this mode (since eventually I also want to fix the issue with the test not just disable it). The failure seem to be some exception or some other reason for an early exit before the test could do anything. https://treeherder.mozilla.org/logviewer.html#?job_id=123423688&repo=mozilla-central&lineNumber=2724
I suspect what happens here is that jsdcov runs in non-e10s for many tests.  I have been working on turning on non-e10s for windows and ran into the exact same failure- I am sure that is what is going on with the specific failure.

jsdcov essentially uses the JS debugger to collect code coverage while the test is running and specific JS related coverage from the browser for that test.  I am not sure if it works on osx/windows- there is some tooling that is required to make everything work smoothly (not unreasonable to make it work though).

tier-1 = backout on all failures - no code coverage should be here ATM
tier-2 = report failures, but do not backout - linux64-ccov and linux64-jsdcov should both be here.
Flags: needinfo?(jmaher)
Gabor, sorry for the late reply. Here's a link to a document that you could look at with some information about the 'ccov' (for c++ and javascript virtual machine code coverage) and 'jsdcov' (for per test JS Debugger code coverage) builds: https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Measuring_Code_Coverage_on_Firefox

You can run the js debugger code coverage collection locally on windows, but it hasn't been tested completely. There's instructions for this at the bottom of the MDN doc that I linked above. I'm not sure about OSX though.

Also note that a problem recently popped up that affects jsdcov and ccov scheduling so we have to run them through 'mach try fuzzy' as shown in bug 1391483 comment 7. (Our docs may or may not be updated soon, depending on the outcome of that bug).
I can't find a bug that states that it should be running so I think this problem is back again... [1]

:jmaher, do you know if this is a bug or if they were enabled on purpose?

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f458045d341be31737018e1968109f3d6271a0fc
Flags: needinfo?(jmaher)
these should be enabled by default, I assume some refactoring messed this up.  we should fix this to help avoid confusion.  :gmeirz, if you cannot figure this out- we can make sure to fix it in person at the all hands.
Flags: needinfo?(jmaher)
Depends on: 1473048
You need to log in before you can comment on or make changes to this bug.