Closed Bug 945095 Opened 8 years ago Closed 7 years ago

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

Categories

(Testing Graveyard :: JSMarionette, defect)

x86
macOS
defect
Not set
normal

Tracking

(b2g-v1.3 affected, b2g-v1.4 fixed)

RESOLVED FIXED
Tracking Status
b2g-v1.3 --- affected
b2g-v1.4 --- fixed

People

(Reporter: yurenju, Assigned: asuth)

References

Details

Attachments

(2 files, 2 obsolete files)

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".
this is the pull request for marionette-js-runner, we need another PR for gaia.
Assignee: nobody → yurenju.mozilla
Status: NEW → ASSIGNED
Attachment #8340907 - Flags: review?(evanxd)
Hi Yuren,

Please make sure the Travis is good before we start to review the patch.
Thanks. :)
Attachment #8340907 - Flags: review?(evanxd)
Duplicate of this bug: 947474
Adding James to the cc list. He own marionette-js-runner.
Evan, Travis is Green on the pull request now ;)
Flags: needinfo?(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.
Flags: needinfo?(evanxd)
Right, I don't know which build I was looking at...
This break tests (see dupe bug 947474). Breaking tests should be blocking everything else.
blocking-b2g: --- → 1.4?
Use REPORTER=spec when launching the tests and you're good for now.
Can we update the README with this workaround until this bug is fixed? Would remove a barrier of entry for newcomers.
I'd rather fix this for good...

Yuren, are you blocked by something? Or is it only that you lack time?
Julien, I'm fighting for refactoring & planning build system, it would be great if anyone can steal this issue :-)
ok thanks :) I'll reset the assignee for now, so that someone that is hurt enough can take it from your patch ;)
Assignee: yurenju.mozilla → nobody
Assignee: nobody → dale
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
Attachment #8360098 - Flags: review?(felash)
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
Attachment #8360098 - Flags: review?(felash)
Wondering why we can't move forward the initial patch from Yuren?
Duplicate of this bug: 960091
(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.
Assignee: dale → bugmail
Attachment #8340907 - Attachment is obsolete: true
Attachment #8360098 - Attachment is obsolete: true
Attachment #8363535 - Flags: review?(felash)
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.
Attachment #8363537 - Flags: review?(felash)
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!
Attachment #8363535 - Flags: review?(felash) → review+
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 :)
Attachment #8363537 - Flags: review?(felash)
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.
(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.
(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 :)
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.
Flags: needinfo?(felash)
done !
Flags: needinfo?(felash)
thanks!  landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/15590
https://github.com/mozilla-b2g/gaia/commit/30895f12aa3037e26ef77cb492b4373be66149c0
Status: ASSIGNED → RESOLVED
blocking-b2g: 1.4? → ---
Closed: 7 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.