Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1124873002:
Mocha adapter for Polymer browser tests. (Closed)
|
Created:
5 years, 7 months ago by michaelpg Modified:
5 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, Kyle Horimoto, Oren Blasberg, Jeremy Klein, apacible Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMocha adapter for Polymer browser tests.
A custom reporter enables mocha in WebUI browser tests. This is
used by a new BrowserTest base class, PolymerTest, to configure
mocha.
Relevant browser_tests sample output: http://pastebin.com/raw.php?i=EeHrmtWH
BUG=482770
Committed: https://crrev.com/8247dfe6962829ae8122df4375099c036e39ac9e
Cr-Commit-Position: refs/heads/master@{#332537}
Patch Set 1 : Split out non-mocha stuff #
Total comments: 6
Patch Set 2 : rebase #Patch Set 3 : #Patch Set 4 : Mocha adapter and Polymer BrowserTest. #
Total comments: 2
Patch Set 5 : add comments #
Total comments: 14
Patch Set 6 : Some fixes #
Total comments: 5
Patch Set 7 : #Patch Set 8 : #
Total comments: 5
Patch Set 9 : override setUp #
Messages
Total messages: 35 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
michaelpg@chromium.org changed reviewers: + jhawkins@chromium.org, stevenjb@chromium.org
Seconda Parte. Mocha provides the async support that test_api.js claims to have but actually doesn't, and has a bunch of useful test lifecycle functions we can't use because we want to run multiple tests as one "browser test" to save cycles. https://codereview.chromium.org/1124873002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1124873002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:210: <include name="IDR_MOCHA_JS" file="..\..\third_party\mocha\mocha.js" flattenhtml="true" type="chrome_html" /> I don't feel good about adding these to browser_resources. What is the alternative? These can't just be GEN_INCLUDE'd in tests because they will fail to eval in the gen step. Mocha requires an actual live browser.
https://codereview.chromium.org/1124873002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/mocha_adapter.js:39: testDone([ should be "else {... }"
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1124873002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1124873002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:210: <include name="IDR_MOCHA_JS" file="..\..\third_party\mocha\mocha.js" flattenhtml="true" type="chrome_html" /> On 2015/05/05 12:23:54, michaelpg wrote: > I don't feel good about adding these to browser_resources. What is the > alternative? > > These can't just be GEN_INCLUDE'd in tests because they will fail to eval in the > gen step. Mocha requires an actual live browser. Yeah, this seems weird, but I don't know what the alternative would be. +dbeam@, +jhawkins@, they may have ideas here. https://codereview.chromium.org/1124873002/diff/80001/third_party/mocha/mocha.js File third_party/mocha/mocha.js (right): https://codereview.chromium.org/1124873002/diff/80001/third_party/mocha/mocha... third_party/mocha/mocha.js:1: ;(function(){ This file should have mocha's coopright header at least, we probably also need licence info and a README. I'm not sure what the process is for this; Dan, James?
https://codereview.chromium.org/1124873002/diff/80001/third_party/mocha/mocha.js File third_party/mocha/mocha.js (right): https://codereview.chromium.org/1124873002/diff/80001/third_party/mocha/mocha... third_party/mocha/mocha.js:1: ;(function(){ Definitely, this is just a placeholder I bower'd manually. I'll talk with Dan and infra about the best way to source Mocha.
https://codereview.chromium.org/1124873002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1124873002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:210: <include name="IDR_MOCHA_JS" file="..\..\third_party\mocha\mocha.js" flattenhtml="true" type="chrome_html" /> On 2015/05/05 22:30:28, stevenjb wrote: > On 2015/05/05 12:23:54, michaelpg wrote: > > I don't feel good about adding these to browser_resources. What is the > > alternative? > > > > These can't just be GEN_INCLUDE'd in tests because they will fail to eval in > the > > gen step. Mocha requires an actual live browser. > > Yeah, this seems weird, but I don't know what the alternative would be. +dbeam@, > +jhawkins@, they may have ideas here. We could exclude these from branding=Chrome builds, so mocha.js isn't included in released versions. But it would still be baked into Chromium, which we don't really want. We could set up a server in the browser test and serve these files through that, but then we run into CSP restrictions. Dan or James, any suggestions or tips?
PTAL. There's only 1 file to be committed now -- the other two are just an example and will be removed before CQ. The reporter now provides a stack trace. The raw output isn't perfect, but it's good enough, and will be improved when we replace js2gtest with something sane. http://pastebin.com/raw.php?i=EeHrmtWH
Patchset #3 (id:110001) has been deleted
Patchset #3 (id:130001) has been deleted
Patchset #4 (id:170001) has been deleted
Now ready for full review & commit. I've removed sample files and added a simple Polymer BrowserTest base class with some helper methods. Polymer tests will derive from this to get everything mocha for free. This is the last CL standing between now and actually testing our Polymer elements!
lgtm https://codereview.chromium.org/1124873002/diff/190001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/190001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:13: * @param {Runner} runner nit: Please provide more details about what runner is and how it's intended to be used.
https://codereview.chromium.org/1124873002/diff/190001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/190001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:13: * @param {Runner} runner On 2015/06/01 20:18:17, James Hawkins wrote: > nit: Please provide more details about what runner is and how it's intended to > be used. Done.
https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:23: passes++; what if a test accidentally passes twice and another never passes? https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:46: link.href = 'chrome://resources/polymer/v0_8/polymer/polymer.html'; set .href after assigning onload and onerror https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:49: }; i assume you're not doing link.onload = done; because it passes a load event? https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:63: setTimeout(function() { setTimeout(fn);? https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:76: function() { done(); }, nit: could "done," work here? https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:81: * Removes all content from the <body>. nit: clearBody or conversely: can we clear .documentElement?
Patchset #6 (id:230001) has been deleted
https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:23: passes++; On 2015/06/02 18:11:16, Dan Beam wrote: > what if a test accidentally passes twice and another never passes? All that junk is handled for free! Mocha throws an error if a test times out or if a test calls done() multiple times. https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:46: link.href = 'chrome://resources/polymer/v0_8/polymer/polymer.html'; On 2015/06/02 18:11:16, Dan Beam wrote: > set .href after assigning onload and onerror Done, but does it really matter? https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:49: }; On 2015/06/02 18:11:16, Dan Beam wrote: > i assume you're not doing link.onload = done; because it passes a load event? Yes, done is basically @param {?Error}, other arguments result in mocha generating an error. https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:63: setTimeout(function() { On 2015/06/02 18:11:16, Dan Beam wrote: > setTimeout(fn);? Removed. https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:76: function() { done(); }, On 2015/06/02 18:11:16, Dan Beam wrote: > nit: could "done," work here? Nope, this callback is passed the load or error event, so the same reasoning applies. FYI: https://github.com/mochajs/mocha/issues/1187 https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:81: * Removes all content from the <body>. On 2015/06/02 18:11:16, Dan Beam wrote: > nit: clearBody or conversely: can we clear .documentElement? Done. This doesn't "unload" HTML templates that were imported by adding a <link> to the head, right? https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:36: message += stack[i] + '\n'; Changed because mocha.js lines can occur first in async hooks like suiteSetup.
does it blend^Wcompile? https://youtu.be/lBUJcD6Ws6s https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:81: * Removes all content from the <body>. On 2015/06/02 19:32:59, michaelpg wrote: > On 2015/06/02 18:11:16, Dan Beam wrote: > > nit: clearBody or conversely: can we clear .documentElement? > > Done. This doesn't "unload" HTML templates that were imported by adding a <link> > to the head, right? yes, it does. that's why I asked. if you want to keep the <head> as is, switch back to body https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:35: if (!stack[i].includes('mocha.js:')) nit: why includes() instead of .indexOf() != -1? we don't use a lot of ES6 features yet...
On 2015/06/02 20:44:33, Dan Beam wrote: > does it blend^Wcompile? eh, just worry about this later
Patchset #7 (id:270001) has been deleted
ptal, thanks! https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1124873002/diff/210001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:81: * Removes all content from the <body>. On 2015/06/02 20:44:32, Dan Beam wrote: > On 2015/06/02 19:32:59, michaelpg wrote: > > On 2015/06/02 18:11:16, Dan Beam wrote: > > > nit: clearBody or conversely: can we clear .documentElement? > > > > Done. This doesn't "unload" HTML templates that were imported by adding a > <link> > > to the head, right? > > yes, it does. that's why I asked. if you want to keep the <head> as is, switch > back to body I see. That would probably be fine as long as the element gets registered, but it's kind of scary. Let's leave it as clearBody and see if we end up needing to add clearDocument. https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:35: if (!stack[i].includes('mocha.js:')) On 2015/06/02 20:44:33, Dan Beam wrote: > nit: why includes() instead of .indexOf() != -1? we don't use a lot of ES6 > features yet... It's just nicer, so why not?
https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:35: if (!stack[i].includes('mocha.js:')) On 2015/06/02 21:30:25, michaelpg wrote: > On 2015/06/02 20:44:33, Dan Beam wrote: > > nit: why includes() instead of .indexOf() != -1? we don't use a lot of ES6 > > features yet... > > It's just nicer, so why not? JSC_INEXISTENT_PROPERTY: Property contains never defined on String at line 1 character 13 alert('asdf'.contains('sdf')); ^
https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... File chrome/test/data/webui/mocha_adapter.js (right): https://codereview.chromium.org/1124873002/diff/250001/chrome/test/data/webui... chrome/test/data/webui/mocha_adapter.js:35: if (!stack[i].includes('mocha.js:')) On 2015/06/02 21:32:27, Dan Beam wrote: > On 2015/06/02 21:30:25, michaelpg wrote: > > On 2015/06/02 20:44:33, Dan Beam wrote: > > > nit: why includes() instead of .indexOf() != -1? we don't use a lot of ES6 > > > features yet... > > > > It's just nicer, so why not? > > JSC_INEXISTENT_PROPERTY: Property contains never defined on String at line 1 > character 13 > alert('asdf'.contains('sdf')); > ^ Acknowledged.
https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:25: browsePreload: 'chrome://chrome-urls/', whatever this is satisfying: can we just turn it off? https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:41: setUp: function() { setUpPage instead? https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:41: setUp: function() { is this supposed to be @overridden? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... e.g. /** @override */ testing.Test.prototype.setUp.call(this); ... rest of code ...
https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:25: browsePreload: 'chrome://chrome-urls/', On 2015/06/02 21:44:50, Dan Beam wrote: > whatever this is satisfying: can we just turn it off? I didn't look too much into this. If we remove the assert, the test just silently fails. Presumably the WebUI is involved in communicating the status of the test. It's something we'll have to tackle, but I don't think it's pressing. https://codereview.chromium.org/1124873002/diff/310001/chrome/test/data/webui... chrome/test/data/webui/polymer_browser_test_base.js:41: setUp: function() { On 2015/06/02 21:44:50, Dan Beam wrote: > is this supposed to be @overridden? > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... > > e.g. > > /** @override */ > > testing.Test.prototype.setUp.call(this); > ... rest of code ... Done.
lgtm
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jhawkins@chromium.org Link to the patchset: https://codereview.chromium.org/1124873002/#ps330001 (title: "override setUp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124873002/330001
Message was sent while issue was closed.
Committed patchset #9 (id:330001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8247dfe6962829ae8122df4375099c036e39ac9e Cr-Commit-Position: refs/heads/master@{#332537} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
