Closed Bug 986206 Opened 9 years ago Closed 9 years ago

MarionetteJS tests don't report filenames on test failure

Categories

(Testing Graveyard :: JSMarionette, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: jgriffin, Assigned: gaye)

References

Details

(Whiteboard: [priority] [p=8])

Attachments

(4 files, 1 obsolete file)

The gaia-integration tests on TBPL were unhidden even though they violate one of the visibility rules, since we know they're urgently needed.

The visibility requirement we're not meeting is outputting test filenames in failing tests, which is described here:  https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#6.29_Outputs_failures_in_a_TBPL-starrable_format

Without filenames, sheriffing is much harder than it should be, because:

- it's often not obvious from failure strings which component a failure should be filed under, e.g., 

11:34:51     INFO -  TEST-UNEXPECTED-FAIL | manipulate display settings "before each" hook

What component should that be filed under?  Who knows!

- a sheriff can't disable a test without grepping through the entire gaia repo to find where this message is coming from

- starring is hard because TBPL will do full text matching against the error string, so the above error message will match any bug that contains any of those strings

Can we please add filename printing to the TEST-UNEXPECTED-FAIL messages in the MarionetteJS logs?
Basically, the failures should look like this:
TEST-UNEXPECTED-FAIL | <test name here> | <failure message>
Evan, do you have time to take a look at this one?
Flags: needinfo?(evanxd)
Whiteboard: [priority]
Hi Dylan,

Sure, I will.

But I'm investigating the Gaia tree closure issue(http://bugzil.la/986325)
After that, I will work on this.
Flags: needinfo?(evanxd)
Hi Jonathan,

We probably could not do that without patching the Mocha testing framework.
See the file name issue for Mocha at https://github.com/visionmedia/mocha/issues/950.

So we might need to patch the Mocha first, or we could have our customized Mocha to fix this bug.

How do you think?
Flags: needinfo?(jgriffin)
I prefer to patch the Mocha.
But it might take much time to fix this bug.
If we could get this fixed in the next couple of weeks, that would be ideal...if patching mocha will take longer than that due to upstream feedback loops, can we fix it with a mocha fork in the meantime?
Flags: needinfo?(jgriffin)
And it would allow to eventually retire the python runner for unit tests too.
Another quick and dirty solution is to add the filenames to all outermost suites' titles.
We need a short-term solution here. It's great if we have a clean fix for long-term but we have to get this up and running asap.
I can try to add this to mocha tomorrow. Then if my patch doesn't get upstreamed very quickly, we can fork mocha for the time being.
Assignee: nobody → gaye
If we would like to have quick solution here, we could just have a fork mocha to add the file name feature. And thanks for Gareth's help.
(In reply to Gareth Aye [:gaye] from comment #10)
> I can try to add this to mocha tomorrow. Then if my patch doesn't get
> upstreamed very quickly, we can fork mocha for the time being.

Please do!
Sent a patch to mocha. If visionmedia doesn't want it, we can fork.
Attachment #8398068 - Attachment description: Link to Github pull-request: https://github.com/visionmedia/mocha/1174 → Link to Github pull-request: https://github.com/visionmedia/mocha/pull/1174
I'm going to implement the mocha-tbpl-reporter part now
Hey :jgriffin,

Would you mind taking a look at the mocha-tbpl-reporter part of this work for me?

Thanks!
Attachment #8398672 - Flags: review?(jgriffin)
Comment on attachment 8398672 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/mocha-tbpl-reporter/pull/6

Great, thanks!
Attachment #8398672 - Flags: review?(jgriffin) → review+
Now onto the change to gaia-node-modules...
gaia-node-modules part https://github.com/mozilla-b2g/gaia-node-modules/commit/f8af4dc9ed87d2167b70bba5857ece130f18d00f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This is great, thanks Gareth for making this happen!
Whiteboard: [priority] → [priority] [p=5]
Target Milestone: --- → 1.4 S4 (28mar)
Whiteboard: [priority] [p=5] → [priority] [p=8]
This doesn't seem to have changed our error reporting on TBPL... see e.g., https://tbpl.mozilla.org/php/getParsedLog.php?id=37087713&tree=Mozilla-Inbound.

Is there something that still needs to happen here?
Sorry yeah I have to update the package.json in gaia...
Actually hub just wrote something to dev-gaia about gaia-node-modules... I may have to read that...
Don't you also need to update gaia-node-modules??
Ah, the main thing you need to do now is update the hash in this file: https://github.com/mozilla-b2g/gaia/blob/master/gaia_node_modules.revision
Package.json needs to match whatever gaia-node-modules is. Not sure why you put a github url into package.json which will fail on tbpl. We need to wait for mocha to publish a package and propagate that through gaia-node-modules first.

Reverted: https://github.com/mozilla-b2g/gaia/commit/fb7b9ecb89dc097b8bd5bb4d74c73033507ef7d9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Why would it fail on tbpl?
Not entirely sure, but my guess it's because we use a git mirror, and it probably doesn't have access to that URL?
Parts of the build relying on non-mozilla servers is discouraged, and in some places actively blocked by the firewall (and hopefully fully blocked in the future).
This is no different than what we're doing with stuff from npm. When we run `npm cache clean && npm install --ignore-scripts`, npm downloads the thing from github. Then when we push up to github, it gets cached just the same as the stuff from npm. When git mirror pulls from github.com/mozilla-b2g/node-modules, it grabs the cached copy. git.mozilla.org doesn't need to talk to visionmedia/mocha...
But I agree that our dependency situation is very convoluted/confusing fwiw
(In reply to Gareth Aye [:gaye] from comment #33)
> This is no different than what we're doing with stuff from npm. When we run
> `npm cache clean && npm install --ignore-scripts`, npm downloads the thing
> from github. 

We should file a bug on fixing this then :-)
Actually we cache those on http://npm-mirror.pub.build.mozilla.org, so we don't hit github (which is fortunate, otherwise the B2G jobs wouldn't be meeting https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy) :-)
Actually, the NPM mirror has been taken down; we never got it working reliably.  We now use static copies of npm modules from the gaia-node-modules repo.

I agree we don't want github references in package.json, since that will lead to flakey behavior in TBPL.
Ah ok. I found references to http://npm-mirror.pub.build.mozilla.org/ in mozharness & presumed we were still using it. Sorry for the confusion! :-)
I think people may misunderstand the issue which is that npm apparently never looks in the local fs for packages which were downloaded from GH. Anyways, waiting on visionmedia to npm publish a version with the patch to mocha.
It could be time to consider forking mocha...
Target Milestone: 1.4 S4 (28mar) → 1.4 S6 (25apr)
If we find they are not being responsive, we can absolutely fork. I wonder if we should send the maintainers a few notes on alternative channels to github to prod them about a version bump.
I don't think we even need to fork gaia. As a stopgap, we can just point the package.json file in gaia-node-modules at the specific commit that accepted gaye's pull request. Something like this:

{
  ..
  "mocha": "https://github.com/visionmedia/mocha/tarball/64396afeefe0927e2ba4bf588642372421b81ed5"
}
The problem is I don't believe that NPM is able to reverse-map those hashes to version numbers. I think if you do this we're going to start downloading the package from github over HTTP (even if it's in the tarball). At least that's what was happening the last time we made this change.
Comment on attachment 8408598 [details] [review]
Github PR, use mocha commit that supports filenames

Ahhh this is what :jgriffin was trying to explain to me today, yet I persisted.

My thought was that since we were no longer using our npm mirror, and that we were flattening the node modules, we should be able to use the tarball url. But I didn't read this bug carefully enough, because comment 40 explains the actual reason we cant use urls.

Sorry for the bugmail spam.
Attachment #8408598 - Attachment is obsolete: true
Attachment #8408598 - Flags: review?(gaye)
Looks like we still don't have a new release from mocha; can we go ahead and fork temporarily to fix this problem?
Flags: needinfo?(gaye)
https://twitter.com/travisjeffery/status/458365486276042752 soon? I made a fork here https://www.npmjs.org/package/mocha-gaia but don't want to flip the switch and then flip back 2 seconds later or even worse maintain our fork lol
Flags: needinfo?(gaye)
Maybe we can exploit heartbleed to publish an update on their behalf!

[This is 100% a joke.]
Target Milestone: 1.4 S6 (25apr) → 1.5 S1 (9may)
Updated mocha-tbpl-reporter in gaia-node-modules... now updating gaia revision of gaia-node-modules...
I think we probably need bug 1001224 to be finished first unfortunately =/
Depends on: 1001224
This should've landed in master as a part of: https://github.com/mozilla-b2g/gaia/commit/68e4ae3bc6af77091635e0b72d831748e6c2a620
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.