945095 - Use same format to set reporter between test-agent & test-integration

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Currently we use two different strings to set reporter between test-agent and gaia-marionette, e.g., "Spec" and "spec"

mocha |require| all reporter with capitalization module names, so we should make gaia-marionette support capitalization reporter name such as "Spec" and "Dot".

Comment 1

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: github PR: part 1 - https://github.com/mozilla-b2g/marionette-js-runner/pull/28

this is the pull request for marionette-js-runner, we need another PR for gaia.

Comment 2

[Evan Tseng [:evanxd]]

Hi Yuren,

Please make sure the Travis is good before we start to review the patch.
Thanks. :)

Comment 4

[Hubert Figuiere [:hub]]

Adding James to the cc list. He own marionette-js-runner.

Comment 5

[Julien Wajsberg [:julienw]]

Evan, Travis is Green on the pull request now ;)

Comment 6

[Evan Tseng [:evanxd]]

Hi Julien,

I don't why it is not green now. https://travis-ci.org/mozilla-b2g/marionette-js-runner/builds/14842407
But let us run again for the patch.

Comment 7

[Julien Wajsberg [:julienw]]

Right, I don't know which build I was looking at...

Comment 8

[Hubert Figuiere [:hub]]

This break tests (see dupe bug 947474). Breaking tests should be blocking everything else.

Comment 9

[Julien Wajsberg [:julienw]]

Use REPORTER=spec when launching the tests and you're good for now.

Comment 10

[Andrew Halberstadt [:ahal]]

Can we update the README with this workaround until this bug is fixed? Would remove a barrier of entry for newcomers.

Comment 11

[Julien Wajsberg [:julienw]]

I'd rather fix this for good...

Yuren, are you blocked by something? Or is it only that you lack time?

Comment 12

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Julien, I'm fighting for refactoring & planning build system, it would be great if anyone can steal this issue :-)

Comment 13

[Julien Wajsberg [:julienw]]

ok thanks :) I'll reset the assignee for now, so that someone that is hurt enough can take it from your patch ;)

Comment 14

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15328

This isnt a fix for using the same reported / underlying filesystem issues so can file as a seperate bug or reopen / reuse the duped one, but just a quick fix so |make test-integration| to work for everyone that keeps trying to use it

Comment 15

[Dale Harvey (:daleharvey)]

Comment on attachment 8360098 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15328

Cancelling review, breaks local unit tests which are somehow different from travis unit tests

Comment 16

[Julien Wajsberg [:julienw]]

Wondering why we can't move forward the initial patch from Yuren?

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Yuren Ju [:yurenju][:小朱] from comment #0)
> Currently we use two different strings to set reporter between test-agent
> and gaia-marionette, e.g., "Spec" and "spec"
> 
> mocha |require| all reporter with capitalization module names, so we should
> make gaia-marionette support capitalization reporter name such as "Spec" and
> "Dot".

I'm looking into this now because this is indeed frustrating.  I think this is backwards and we should fix js-test-agent to use lower-case reporter names.

My rationale is that this is consistent with the code in mocha itself which I include here:
===
Mocha.prototype.reporter = function(reporter){
  if ('function' == typeof reporter) {
    this._reporter = reporter;
  } else {
    reporter = reporter || 'dot';
    try {
      this._reporter = require('./reporters/' + reporter);
    } catch (err) {
      this._reporter = require(reporter);
    }
    if (!this._reporter) throw new Error('invalid reporter "' + reporter + '"');
  }
  return this;
};
===

Also, we have a TBPL reporter https://github.com/mozilla-b2g/mocha-tbpl-reporter that advises using "--reporter mocha-tbpl-reporter" to enable it.  A capitalization mapping would be awkward weird for this.

The actual capitalization used inside the module is irrelevant since the code inside mocha/reports/dot.js for example is "exports = module.exports = Dot;"


My proposed solution is thus to:

- Modify js-test-agent/lib/node/bin/test.js (and anything like it) to continue to use the existing CamelCase option as a first priority, then fail-over to the mocha canonical lowercase lookup mapping.  js-test-agent currently appears to not support mocha-tbpl-reporter, which this solution should address.

- Change our Makefile to REPORTER=spec, the choice of a new generation.

I am stealing the bug.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Support lowercase names in js-test-agent

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Use lower-case REPORTER for both integration and js-test agent

Obviously, the actual commit would have an NPM version number instead of a git hash.  Asking Julien for review since this is basically just a js-test-agent thing.

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8363535 [details] [review]
Support lowercase names in js-test-agent

r=me

The rationale is sane, let's do this!

Thanks a lot Andrew!

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 8363537 [details] [review]
Use lower-case REPORTER for both integration and js-test agent

I don't know how to publish to the npm registry yet but just bump the version to 0.15.0 and let's wait for a green travis.

Ping me when ready :)

Comment 23

[Hubert Figuiere [:hub]]

You need to have the proper rights on the module. Then it is just a matter of "npm publish".

You can find the list of  maintainers there:

https://npmjs.org/package/test-agent

Kevin or Mike should be able to help.

BTW the r+ patch does bump to 0.14.1. Make sure to change that before merging.

Comment 24

[Julien Wajsberg [:julienw]]

(In reply to Hubert Figuiere [:hub] from comment #23)

> BTW the r+ patch does bump to 0.14.1. Make sure to change that before
> merging.

Yep, I said it in a comment on github.

Comment 25

[Julien Wajsberg [:julienw]]

(In reply to Hubert Figuiere [:hub] from comment #23)
> You need to have the proper rights on the module. Then it is just a matter
> of "npm publish".
> 
> You can find the list of  maintainers there:
> 
> https://npmjs.org/package/test-agent
> 
> Kevin or Mike should be able to help.
> 

James just added me too, so it'll be fine :)

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Okay, I landed the js-test-agent fix after confirming that gaia was fine with those changes via a github git URL in npm.  (The python tests had an error, and there were some failures in the b2g version of the unit tests, but nothing related to the reporter.)

That travis run is here: https://travis-ci.org/mozilla-b2g/gaia/builds/17463781

I have updated the https://github.com/mozilla-b2g/gaia/pull/15590 to just use version 0.15.0.  Julien or someone else needs to upload the 0.15.0 release first, of course.  needinfo-ing Julien to this end.

Comment 27

[Julien Wajsberg [:julienw]]

done !

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

thanks!  landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/15590
https://github.com/mozilla-b2g/gaia/commit/30895f12aa3037e26ef77cb492b4373be66149c0