Issue 29866577: Issue 6887 - add Chrome to "npm test" (Closed)
|
Created:
Aug. 27, 2018, 3:09 p.m. by tlucas Modified:
Sept. 1, 2018, 3:30 p.m. Reviewers:
Sebastian Noack CC:
kzar, hub, sergei Visibility:
Public. |
DescriptionIssue 6887 - add Chrome to "npm test"
Patch Set 1 #
Total comments: 38
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : Removed unused this.platform #
MessagesTotal messages: 14
https://codereview.adblockplus.org/29866577/diff/29866578/package.json File package.json (right): https://codereview.adblockplus.org/29866577/diff/29866578/package.json#newcode10 package.json:10: "The chromedriver version was pinned to that exact version, due tu in-", Typo: It's "due to incompatibilities". https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome.js File test/browser/chrome.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:1: /* For the directory name "browsers" (plural) seems more appropriate. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:18: /* eslint-env mocha */ Is this necessary? I don't see any describe()/it/etc in here. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:22: const CHROMIUM_REVISION = 508578; We might want to add a comment which Chrome version this refers to. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:36: return ensureChromium(CHROMIUM_REVISION).then(chromiumPath => Are we going to download a Google Chrome or Chromium build? The name of this function and the the module it is imported from suggests the latter. If that is true we probably should also call this script chromium.js (no chrome.js). https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:38: let devenv = path.resolve("./devenv.chrome"); Is path.resolve() necessary here? https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:45: driver = new webdriver.Builder() If you just return driver here, that would make the next callback the "driver" variable redundant. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/firefo... File test/browser/firefox.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/firefo... test/browser/firefox.js:1: /* See my comments for chrome.js, almost all of them apply here as well. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js File test/run.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode1 test/run.js:1: /* FWIW, I think all.js is a slightly better name for this module. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode26 test/run.js:26: let testFiles = glob.sync("./test/cases/*.js"); Nit: IMO, the code reads better if we inline this call below. Yes, then it's evaluated twice, but I doubt that causes any measurable overhead. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode30 test/run.js:30: let module = require(path.resolve(browser)); Is path.resolve() necessary here? https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode31 test/run.js:31: let buildCommand = `python build.py devenv -t ${module.platform}`; Nit: Since this variable is only used once, inline it below? https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode41 test/run.js:41: let buildDevenv = new Promise((resolve, reject) => This should happen within before(). https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode45 test/run.js:45: if (error) reject(stderr); In case of an error, we might rather want to forward the output to stderr of the current process and then reject the promise with the provided error. (This would equivalent to not catching STDERR in the first place when using the subprocess module in Python.) Also that way we make sure, that the error is printed an readable format, and a proper error object is propagated. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode71 test/run.js:71: it(testModule.title, () => I wonder, would it work if we call it() on the top-level inside the module so that we just need to call require() here and don't need this wrapper?
Patch Set 2: * Address Sebastian's comments. https://codereview.adblockplus.org/29866577/diff/29866578/package.json File package.json (right): https://codereview.adblockplus.org/29866577/diff/29866578/package.json#newcode10 package.json:10: "The chromedriver version was pinned to that exact version, due tu in-", On 2018/08/27 19:56:15, Sebastian Noack wrote: > Typo: It's "due to incompatibilities". That's exactly what's written here?... Or do you mean the line breaking? https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome.js File test/browser/chrome.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:1: /* On 2018/08/27 19:56:15, Sebastian Noack wrote: > For the directory name "browsers" (plural) seems more appropriate. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:18: /* eslint-env mocha */ On 2018/08/27 19:56:15, Sebastian Noack wrote: > Is this necessary? I don't see any describe()/it/etc in here. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:22: const CHROMIUM_REVISION = 508578; On 2018/08/27 19:56:15, Sebastian Noack wrote: > We might want to add a comment which Chrome version this refers to. Copied the comment from adblockpluscore/test/runners/chromium_process.js https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:36: return ensureChromium(CHROMIUM_REVISION).then(chromiumPath => On 2018/08/27 19:56:15, Sebastian Noack wrote: > Are we going to download a Google Chrome or Chromium build? The name of this > function and the the module it is imported from suggests the latter. If that is > true we probably should also call this script chromium.js (no chrome.js). Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:38: let devenv = path.resolve("./devenv.chrome"); On 2018/08/27 19:56:15, Sebastian Noack wrote: > Is path.resolve() necessary here? Yes, "load-extension" (below) requires an absolute path. (The same applies to Firefox's extension loading) https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/chrome... test/browser/chrome.js:45: driver = new webdriver.Builder() On 2018/08/27 19:56:15, Sebastian Noack wrote: > If you just return driver here, that would make the next callback the "driver" > variable redundant. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/firefo... File test/browser/firefox.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/browser/firefo... test/browser/firefox.js:1: /* On 2018/08/27 19:56:15, Sebastian Noack wrote: > See my comments for chrome.js, almost all of them apply here as well. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js File test/run.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode1 test/run.js:1: /* On 2018/08/27 19:56:16, Sebastian Noack wrote: > FWIW, I think all.js is a slightly better name for this module. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode26 test/run.js:26: let testFiles = glob.sync("./test/cases/*.js"); On 2018/08/27 19:56:16, Sebastian Noack wrote: > Nit: IMO, the code reads better if we inline this call below. Yes, then it's > evaluated twice, but I doubt that causes any measurable overhead. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode30 test/run.js:30: let module = require(path.resolve(browser)); On 2018/08/27 19:56:16, Sebastian Noack wrote: > Is path.resolve() necessary here? Well, "./test/browsers/firefox.js" can't be implicitly resolved from there. What's the alternative? https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode31 test/run.js:31: let buildCommand = `python build.py devenv -t ${module.platform}`; On 2018/08/27 19:56:16, Sebastian Noack wrote: > Nit: Since this variable is only used once, inline it below? Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode41 test/run.js:41: let buildDevenv = new Promise((resolve, reject) => On 2018/08/27 19:56:16, Sebastian Noack wrote: > This should happen within before(). Moved it. To clarify, fmi - doesn't the line below: " before(() => buildDevenv.then(() =>" Cause this to only be executed when before() is called? Or is the Promise started on construction? https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode45 test/run.js:45: if (error) reject(stderr); On 2018/08/27 19:56:15, Sebastian Noack wrote: > In case of an error, we might rather want to forward the output to stderr of the > current process and then reject the promise with the provided error. > > (This would equivalent to not catching STDERR in the first place when using the > subprocess module in Python.) > > Also that way we make sure, that the error is printed an readable format, and a > proper error object is propagated. Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode71 test/run.js:71: it(testModule.title, () => On 2018/08/27 19:56:15, Sebastian Noack wrote: > I wonder, would it work if we call it() on the top-level inside the module so > that we just need to call require() here and don't need this wrapper? In theory yes - How would you pass "driver" and "origin"?
https://codereview.adblockplus.org/29866577/diff/29866578/package.json File package.json (right): https://codereview.adblockplus.org/29866577/diff/29866578/package.json#newcode10 package.json:10: "The chromedriver version was pinned to that exact version, due tu in-", On 2018/08/28 08:18:13, tlucas wrote: > On 2018/08/27 19:56:15, Sebastian Noack wrote: > > Typo: It's "due to incompatibilities". > > That's exactly what's written here?... Or do you mean the line breaking? It's not. What's written here is "due tu in-compatibilities" ^ ^ https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js File test/run.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode30 test/run.js:30: let module = require(path.resolve(browser)); On 2018/08/28 08:18:14, tlucas wrote: > On 2018/08/27 19:56:16, Sebastian Noack wrote: > > Is path.resolve() necessary here? > > Well, "./test/browsers/firefox.js" can't be implicitly resolved from there. > What's the alternative? Right, require() expects a path, relative to the current script. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode41 test/run.js:41: let buildDevenv = new Promise((resolve, reject) => On 2018/08/28 08:18:14, tlucas wrote: > On 2018/08/27 19:56:16, Sebastian Noack wrote: > > This should happen within before(). > > Moved it. > To clarify, fmi - doesn't the line below: > > " before(() => buildDevenv.then(() =>" > > Cause this to only be executed when before() is called? Or is the Promise > started on construction? > Nope. This is not a function (that is called). You were essentially calling exec() right away. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode71 test/run.js:71: it(testModule.title, () => On 2018/08/28 08:18:14, tlucas wrote: > On 2018/08/27 19:56:15, Sebastian Noack wrote: > > I wonder, would it work if we call it() on the top-level inside the module so > > that we just need to call require() here and don't need this wrapper? > > In theory yes - How would you pass "driver" and "origin"? From looking at other examples using Mocha, it seems instead of using a variable in the shared closure, you can leverage a shared context (i.e. "this"): before(function() { return somePromise.then(result => { this.result = result; }); }); it(function() { somePromise.then(() => { doSomethingWith(this.result); } }); Note that it's important to use the function statement for the callback passed to before() and it(), while using arrow functions for any nested callback. That is because arrow functions ignore any context provided, but bind "this" to the context of the parent scope. https://codereview.adblockplus.org/29866577/diff/29867558/package.json File package.json (left): https://codereview.adblockplus.org/29866577/diff/29867558/package.json#oldcode22 package.json:22: "pretest": "python build.py devenv -t gecko", As mentioned on the other review, we might want to call ensure_depdencies.py here. Previously, it was implicitly called by build.py, but at the time we call build.py now, we have already imported some code from adblockpluscore. https://codereview.adblockplus.org/29866577/diff/29867558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29867558/test/all.js#newcode46 test/all.js:46: reject(stderr); It seems you got it the wrong way around. You want to log the output you got on stderr and propagate the error argument down the promise chain. https://codereview.adblockplus.org/29866577/diff/29867558/test/all.js#newcode51 test/all.js:51: ).then(() => Nit: The indention is off here. Closing parenthesis go on a line with the same level of indentation as the line with the respective opening parenthesis. Then the newline is redundant as well. https://codereview.adblockplus.org/29866577/diff/29867558/test/browsers/chrom... File test/browsers/chromium.js (right): https://codereview.adblockplus.org/29866577/diff/29867558/test/browsers/chrom... test/browsers/chromium.js:39: let devenv = path.resolve("./devenv.chrome"); I notice a pattern here. For both browsers we hard-code "./devenv.<platform>" and then we resolve it. How about having the calling code pass in "path.resolve(`./devenv/${module.platform}`)" (in order to DRY)?
https://codereview.adblockplus.org/29866577/diff/29866578/package.json File package.json (right): https://codereview.adblockplus.org/29866577/diff/29866578/package.json#newcode10 package.json:10: "The chromedriver version was pinned to that exact version, due tu in-", On 2018/08/28 14:44:57, Sebastian Noack wrote: > On 2018/08/28 08:18:13, tlucas wrote: > > On 2018/08/27 19:56:15, Sebastian Noack wrote: > > > Typo: It's "due to incompatibilities". > > > > That's exactly what's written here?... Or do you mean the line breaking? > > It's not. What's written here is "due tu in-compatibilities" > ^ ^ Done. https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js File test/run.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode71 test/run.js:71: it(testModule.title, () => On 2018/08/28 14:44:58, Sebastian Noack wrote: > On 2018/08/28 08:18:14, tlucas wrote: > > On 2018/08/27 19:56:15, Sebastian Noack wrote: > > > I wonder, would it work if we call it() on the top-level inside the module > so > > > that we just need to call require() here and don't need this wrapper? > > > > In theory yes - How would you pass "driver" and "origin"? > > From looking at other examples using Mocha, it seems instead of using a variable > in the shared closure, you can leverage a shared context (i.e. "this"): > > before(function() > { > return somePromise.then(result => > { > this.result = result; > }); > }); > > it(function() > { > somePromise.then(() => > { > doSomethingWith(this.result); > } > }); > > Note that it's important to use the function statement for the callback passed > to before() and it(), while using arrow functions for any nested callback. That > is because arrow functions ignore any context provided, but bind "this" to the > context of the parent scope. Thanks for clarifying. But i don't see, how we'd currently benefit from that. https://codereview.adblockplus.org/29866577/diff/29867558/package.json File package.json (left): https://codereview.adblockplus.org/29866577/diff/29867558/package.json#oldcode22 package.json:22: "pretest": "python build.py devenv -t gecko", On 2018/08/28 14:44:58, Sebastian Noack wrote: > As mentioned on the other review, we might want to call ensure_depdencies.py > here. Previously, it was implicitly called by build.py, but at the time we call > build.py now, we have already imported some code from adblockpluscore. Done. https://codereview.adblockplus.org/29866577/diff/29867558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29867558/test/all.js#newcode46 test/all.js:46: reject(stderr); On 2018/08/28 14:44:58, Sebastian Noack wrote: > It seems you got it the wrong way around. You want to log the output you got on > stderr and propagate the error argument down the promise chain. Done. https://codereview.adblockplus.org/29866577/diff/29867558/test/all.js#newcode51 test/all.js:51: ).then(() => On 2018/08/28 14:44:58, Sebastian Noack wrote: > Nit: The indention is off here. Closing parenthesis go on a line with the same > level of indentation as the line with the respective opening parenthesis. Then > the newline is redundant as well. re indented here and above (at "new Promise") https://codereview.adblockplus.org/29866577/diff/29867558/test/browsers/chrom... File test/browsers/chromium.js (right): https://codereview.adblockplus.org/29866577/diff/29867558/test/browsers/chrom... test/browsers/chromium.js:39: let devenv = path.resolve("./devenv.chrome"); On 2018/08/28 14:44:58, Sebastian Noack wrote: > I notice a pattern here. For both browsers we hard-code "./devenv.<platform>" > and then we resolve it. How about having the calling code pass in > "path.resolve(`./devenv/${module.platform}`)" (in order to DRY)? Done.
https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js File test/run.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode71 test/run.js:71: it(testModule.title, () => On 2018/08/29 09:16:53, tlucas wrote: > On 2018/08/28 14:44:58, Sebastian Noack wrote: > > On 2018/08/28 08:18:14, tlucas wrote: > > > On 2018/08/27 19:56:15, Sebastian Noack wrote: > > > > I wonder, would it work if we call it() on the top-level inside the module > > so > > > > that we just need to call require() here and don't need this wrapper? > > > > > > In theory yes - How would you pass "driver" and "origin"? > > > > From looking at other examples using Mocha, it seems instead of using a > variable > > in the shared closure, you can leverage a shared context (i.e. "this"): > > > > before(function() > > { > > return somePromise.then(result => > > { > > this.result = result; > > }); > > }); > > > > it(function() > > { > > somePromise.then(() => > > { > > doSomethingWith(this.result); > > } > > }); > > > > Note that it's important to use the function statement for the callback passed > > to before() and it(), while using arrow functions for any nested callback. > That > > is because arrow functions ignore any context provided, but bind "this" to the > > context of the parent scope. > > Thanks for clarifying. But i don't see, how we'd currently benefit from that. Well, it would be less code here, less boilerplate in test/wrappers/*, the scripts could contain more than one test case (which may, or may not, be desired in the future), and the code will be easier to understand if you know Mocha. https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode50 test/all.js:50: .then(d => Nit: The indentation seems a little inconsistent. Perhaps change it like this: before(() => new Promise((resolve, reject) => { ... }).then(() => ... ).then(d => ... ) https://codereview.adblockplus.org/29866577/diff/29868558/test/browsers/firef... File test/browsers/firefox.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/browsers/firef... test/browsers/firefox.js:51: return driver; Nit: I think it reads a little better with a blank line above.
https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode37 test/all.js:37: before(() => new Promise((resolve, reject) => I just noticed, getting the driver and setting up the devenv can actually happen in parallel: before() => Promise.all([ module.getDriver(path.resolve(`./devenv.${module.platform}`), new Promise((resolve, reject) => ... ) ]).then(([d]) => { ... })
https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js File test/run.js (right): https://codereview.adblockplus.org/29866577/diff/29866578/test/run.js#newcode71 test/run.js:71: it(testModule.title, () => On 2018/08/29 15:22:12, Sebastian Noack wrote: > On 2018/08/29 09:16:53, tlucas wrote: > > On 2018/08/28 14:44:58, Sebastian Noack wrote: > > > On 2018/08/28 08:18:14, tlucas wrote: > > > > On 2018/08/27 19:56:15, Sebastian Noack wrote: > > > > > I wonder, would it work if we call it() on the top-level inside the > module > > > so > > > > > that we just need to call require() here and don't need this wrapper? > > > > > > > > In theory yes - How would you pass "driver" and "origin"? > > > > > > From looking at other examples using Mocha, it seems instead of using a > > variable > > > in the shared closure, you can leverage a shared context (i.e. "this"): > > > > > > before(function() > > > { > > > return somePromise.then(result => > > > { > > > this.result = result; > > > }); > > > }); > > > > > > it(function() > > > { > > > somePromise.then(() => > > > { > > > doSomethingWith(this.result); > > > } > > > }); > > > > > > Note that it's important to use the function statement for the callback > passed > > > to before() and it(), while using arrow functions for any nested callback. > > That > > > is because arrow functions ignore any context provided, but bind "this" to > the > > > context of the parent scope. > > > > Thanks for clarifying. But i don't see, how we'd currently benefit from that. > > Well, it would be less code here, less boilerplate in test/wrappers/*, the > scripts could contain more than one test case (which may, or may not, be desired > in the future), and the code will be easier to understand if you know Mocha. This actually works pretty nicely. After finding out about require.cache at least, done. https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode37 test/all.js:37: before(() => new Promise((resolve, reject) => On 2018/08/30 02:02:40, Sebastian Noack wrote: > I just noticed, getting the driver and setting up the devenv can actually happen > in parallel: > > before() => Promise.all([ > module.getDriver(path.resolve(`./devenv.${module.platform}`), > new Promise((resolve, reject) => > ... > ) > ]).then(([d]) => > { > ... > }) Correct me if i'm wrong - but couldn't it then happen, that getDriver() tries to load a not yet existent devenv? https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode50 test/all.js:50: .then(d => On 2018/08/29 15:22:12, Sebastian Noack wrote: > Nit: The indentation seems a little inconsistent. Perhaps change it like this: > > before(() => new Promise((resolve, reject) => > { > ... > }).then(() => > ... > ).then(d => > ... > ) Should be resolved now. https://codereview.adblockplus.org/29866577/diff/29868558/test/browsers/firef... File test/browsers/firefox.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/browsers/firef... test/browsers/firefox.js:51: return driver; On 2018/08/29 15:22:12, Sebastian Noack wrote: > Nit: I think it reads a little better with a blank line above. Done.
https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode37 test/all.js:37: before(() => new Promise((resolve, reject) => On 2018/09/01 09:38:31, tlucas wrote: > On 2018/08/30 02:02:40, Sebastian Noack wrote: > > I just noticed, getting the driver and setting up the devenv can actually > happen > > in parallel: > > > > before() => Promise.all([ > > module.getDriver(path.resolve(`./devenv.${module.platform}`), > > new Promise((resolve, reject) => > > ... > > ) > > ]).then(([d]) => > > { > > ... > > }) > > Correct me if i'm wrong - but couldn't it then happen, that getDriver() tries to > load a not yet existent devenv? My bad, you are right. However, what we can do, and I like that idea, is splitting out the part that makes sure the browser is there (this is the expensive part on first run) and parallelize it with creating the devenv. Then browsers/chromium.js for example would look like this: exports.ensureBrowser = function() { return ensureChromium(CHROMIUM_REVISION); }; exports.getDriver = function(browserBinary, devenvPathAbsolute) { let options = new chrome.Options() .setChromeBinaryPath(browserBinary) .addArguments("--no-sandbox") .addArguments(`load-extension=${devenvPathAbsolute}`); return new webdriver.Builder() .forBrowser("chrome") .setChromeOptions(options) .build(); }; And here we get this: before(function() { return Promise.all([ module.ensureBrowser(), new Promise((resolve, reject) => ... ) ]).then([browserBinary] => { this.driver = module.getDriver( browserBinary, path.resolve(`./devenv.${module.platform}`) ); return this.driver.wait(() => this.driver.getAllWindowHandles().then(handles => handles[1]) ); }) https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js#newcode51 test/all.js:51: this.driver = d; Nit: The reason we called the argument "d" (rather than "driver") was to avoid a name conflict. But since we assign to this.driver now, we can spell out "driver" again. Same below for "o" => "origin". https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js#newcode69 test/all.js:69: delete require.cache[require.resolve(path.resolve(file))]; Why would they already be in cache? Isn't this the first time we import them? https://codereview.adblockplus.org/29866577/diff/29871558/test/wrappers/qunit.js File test/wrappers/qunit.js (right): https://codereview.adblockplus.org/29866577/diff/29871558/test/wrappers/qunit... test/wrappers/qunit.js:18: /* eslint-env mocha */ Since there are now multiple files requiring a mocha environment, I'd rather create a test/.eslintrc.json file (similar to qunit/.eslintrc.json) and remove those inline configuration comments. https://codereview.adblockplus.org/29866577/diff/29871558/test/wrappers/qunit... test/wrappers/qunit.js:23: const {By, until} = webdriver; Nit: Unused imports? I don't see webdriver, By and until being used below.
Patch Set 5: * Address Sebastian's comments https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode37 test/all.js:37: before(() => new Promise((resolve, reject) => On 2018/09/01 10:42:47, Sebastian Noack wrote: > On 2018/09/01 09:38:31, tlucas wrote: > > On 2018/08/30 02:02:40, Sebastian Noack wrote: > > > I just noticed, getting the driver and setting up the devenv can actually > > happen > > > in parallel: > > > > > > before() => Promise.all([ > > > module.getDriver(path.resolve(`./devenv.${module.platform}`), > > > new Promise((resolve, reject) => > > > ... > > > ) > > > ]).then(([d]) => > > > { > > > ... > > > }) > > > > Correct me if i'm wrong - but couldn't it then happen, that getDriver() tries > to > > load a not yet existent devenv? > > My bad, you are right. However, what we can do, and I like that idea, is > splitting out the part that makes sure the browser is there (this is the > expensive part on first run) and parallelize it with creating the devenv. Then > browsers/chromium.js for example would look like this: > > exports.ensureBrowser = function() > { > return ensureChromium(CHROMIUM_REVISION); > }; > > exports.getDriver = function(browserBinary, devenvPathAbsolute) > { > let options = new chrome.Options() > .setChromeBinaryPath(browserBinary) > .addArguments("--no-sandbox") > .addArguments(`load-extension=${devenvPathAbsolute}`); > > return new webdriver.Builder() > .forBrowser("chrome") > .setChromeOptions(options) > .build(); > }; > > And here we get this: > > before(function() > { > return Promise.all([ > module.ensureBrowser(), > new Promise((resolve, reject) => > ... > ) > ]).then([browserBinary] => > { > this.driver = module.getDriver( > browserBinary, > path.resolve(`./devenv.${module.platform}`) > ); > > return this.driver.wait(() => > this.driver.getAllWindowHandles().then(handles => handles[1]) > ); > }) Done (almost, nodejs complained about an unexpected token in ]).then([browserBinary] => ^ ) https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js#newcode51 test/all.js:51: this.driver = d; On 2018/09/01 10:42:47, Sebastian Noack wrote: > Nit: The reason we called the argument "d" (rather than "driver") was to avoid a > name conflict. But since we assign to this.driver now, we can spell out "driver" > again. Same below for "o" => "origin". Done. https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js#newcode69 test/all.js:69: delete require.cache[require.resolve(path.resolve(file))]; On 2018/09/01 10:42:47, Sebastian Noack wrote: > Why would they already be in cache? Isn't this the first time we import them? We (want to) require them 2 times currently, once for Firefox, once for Chrome, so that it() is actually executed 2 times. You could argue that deleting this from the cache is only necessary after it was once required, but that would add some boilerplate here. https://codereview.adblockplus.org/29866577/diff/29871558/test/wrappers/qunit.js File test/wrappers/qunit.js (right): https://codereview.adblockplus.org/29866577/diff/29871558/test/wrappers/qunit... test/wrappers/qunit.js:18: /* eslint-env mocha */ On 2018/09/01 10:42:47, Sebastian Noack wrote: > Since there are now multiple files requiring a mocha environment, I'd rather > create a test/.eslintrc.json file (similar to qunit/.eslintrc.json) and remove > those inline configuration comments. Done. https://codereview.adblockplus.org/29866577/diff/29871558/test/wrappers/qunit... test/wrappers/qunit.js:23: const {By, until} = webdriver; On 2018/09/01 10:42:47, Sebastian Noack wrote: > Nit: Unused imports? I don't see webdriver, By and until being used below. You are right about "webdriver",however "By" and "until" are used in e.g. line 30: this.driver.wait(until.elementLocated(By.id("qunit-testresult"))) ^ ^ Done.
https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode37 test/all.js:37: before(() => new Promise((resolve, reject) => On 2018/09/01 11:54:23, tlucas wrote: > On 2018/09/01 10:42:47, Sebastian Noack wrote: > > On 2018/09/01 09:38:31, tlucas wrote: > > > On 2018/08/30 02:02:40, Sebastian Noack wrote: > > > > I just noticed, getting the driver and setting up the devenv can actually > > > happen > > > > in parallel: > > > > > > > > before() => Promise.all([ > > > > module.getDriver(path.resolve(`./devenv.${module.platform}`), > > > > new Promise((resolve, reject) => > > > > ... > > > > ) > > > > ]).then(([d]) => > > > > { > > > > ... > > > > }) > > > > > > Correct me if i'm wrong - but couldn't it then happen, that getDriver() > tries > > to > > > load a not yet existent devenv? > > > > My bad, you are right. However, what we can do, and I like that idea, is > > splitting out the part that makes sure the browser is there (this is the > > expensive part on first run) and parallelize it with creating the devenv. Then > > browsers/chromium.js for example would look like this: > > > > exports.ensureBrowser = function() > > { > > return ensureChromium(CHROMIUM_REVISION); > > }; > > > > exports.getDriver = function(browserBinary, devenvPathAbsolute) > > { > > let options = new chrome.Options() > > .setChromeBinaryPath(browserBinary) > > .addArguments("--no-sandbox") > > .addArguments(`load-extension=${devenvPathAbsolute}`); > > > > return new webdriver.Builder() > > .forBrowser("chrome") > > .setChromeOptions(options) > > .build(); > > }; > > > > And here we get this: > > > > before(function() > > { > > return Promise.all([ > > module.ensureBrowser(), > > new Promise((resolve, reject) => > > ... > > ) > > ]).then([browserBinary] => > > { > > this.driver = module.getDriver( > > browserBinary, > > path.resolve(`./devenv.${module.platform}`) > > ); > > > > return this.driver.wait(() => > > this.driver.getAllWindowHandles().then(handles => handles[1]) > > ); > > }) > > Done (almost, nodejs complained about an unexpected token in > > ]).then([browserBinary] => > ^ > ) Right, you have to use parenthesis if you want to destructure any elements in the arguments list of an arrow function: ]).then(([browserBinary]) => https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29871558/test/all.js#newcode69 test/all.js:69: delete require.cache[require.resolve(path.resolve(file))]; On 2018/09/01 11:54:24, tlucas wrote: > On 2018/09/01 10:42:47, Sebastian Noack wrote: > > Why would they already be in cache? Isn't this the first time we import them? > > We (want to) require them 2 times currently, once for Firefox, once for Chrome, > so that it() is actually executed 2 times. > > You could argue that deleting this from the cache is only necessary after it was > once required, but that would add some boilerplate here. Acknowledged. https://codereview.adblockplus.org/29866577/diff/29871566/test/.eslintrc.json File test/.eslintrc.json (left): https://codereview.adblockplus.org/29866577/diff/29871566/test/.eslintrc.json... test/.eslintrc.json:1: { Huh? It looks above that you moved this file from the quint folder? https://codereview.adblockplus.org/29866577/diff/29871566/test/browsers/chrom... File test/browsers/chromium.js (right): https://codereview.adblockplus.org/29866577/diff/29871566/test/browsers/chrom... test/browsers/chromium.js:39: exports.getDriver = function(browserBinary, devenvPathAbsolute) Nit: I don't think that the path is absolute is an important enough detail to bloat the argument name. I'd just call it "devenvPath".
https://codereview.adblockplus.org/29866577/diff/29871566/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29871566/test/all.js#newcode73 test/all.js:73: require(path.resolve(file)); You call path.resolve() twice here. Perhaps use a variable?
https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29868558/test/all.js#newcode37 test/all.js:37: before(() => new Promise((resolve, reject) => On 2018/09/01 12:26:33, Sebastian Noack wrote: > On 2018/09/01 11:54:23, tlucas wrote: > > On 2018/09/01 10:42:47, Sebastian Noack wrote: > > > On 2018/09/01 09:38:31, tlucas wrote: > > > > On 2018/08/30 02:02:40, Sebastian Noack wrote: > > > > > I just noticed, getting the driver and setting up the devenv can > actually > > > > happen > > > > > in parallel: > > > > > > > > > > before() => Promise.all([ > > > > > module.getDriver(path.resolve(`./devenv.${module.platform}`), > > > > > new Promise((resolve, reject) => > > > > > ... > > > > > ) > > > > > ]).then(([d]) => > > > > > { > > > > > ... > > > > > }) > > > > > > > > Correct me if i'm wrong - but couldn't it then happen, that getDriver() > > tries > > > to > > > > load a not yet existent devenv? > > > > > > My bad, you are right. However, what we can do, and I like that idea, is > > > splitting out the part that makes sure the browser is there (this is the > > > expensive part on first run) and parallelize it with creating the devenv. > Then > > > browsers/chromium.js for example would look like this: > > > > > > exports.ensureBrowser = function() > > > { > > > return ensureChromium(CHROMIUM_REVISION); > > > }; > > > > > > exports.getDriver = function(browserBinary, devenvPathAbsolute) > > > { > > > let options = new chrome.Options() > > > .setChromeBinaryPath(browserBinary) > > > .addArguments("--no-sandbox") > > > .addArguments(`load-extension=${devenvPathAbsolute}`); > > > > > > return new webdriver.Builder() > > > .forBrowser("chrome") > > > .setChromeOptions(options) > > > .build(); > > > }; > > > > > > And here we get this: > > > > > > before(function() > > > { > > > return Promise.all([ > > > module.ensureBrowser(), > > > new Promise((resolve, reject) => > > > ... > > > ) > > > ]).then([browserBinary] => > > > { > > > this.driver = module.getDriver( > > > browserBinary, > > > path.resolve(`./devenv.${module.platform}`) > > > ); > > > > > > return this.driver.wait(() => > > > this.driver.getAllWindowHandles().then(handles => handles[1]) > > > ); > > > }) > > > > Done (almost, nodejs complained about an unexpected token in > > > > ]).then([browserBinary] => > > ^ > > ) > > Right, you have to use parenthesis if you want to destructure any elements in > the arguments list of an arrow function: > > ]).then(([browserBinary]) => Done. https://codereview.adblockplus.org/29866577/diff/29871566/test/.eslintrc.json File test/.eslintrc.json (left): https://codereview.adblockplus.org/29866577/diff/29871566/test/.eslintrc.json... test/.eslintrc.json:1: { On 2018/09/01 12:26:33, Sebastian Noack wrote: > Huh? It looks above that you moved this file from the quint folder? I wondered as well - but the file is still there (and rietveld doesn't show it as moved / deleted) https://codereview.adblockplus.org/29866577/diff/29871566/test/all.js File test/all.js (right): https://codereview.adblockplus.org/29866577/diff/29871566/test/all.js#newcode73 test/all.js:73: require(path.resolve(file)); On 2018/09/01 12:28:10, Sebastian Noack wrote: > You call path.resolve() twice here. Perhaps use a variable? Done. https://codereview.adblockplus.org/29866577/diff/29871566/test/browsers/chrom... File test/browsers/chromium.js (right): https://codereview.adblockplus.org/29866577/diff/29871566/test/browsers/chrom... test/browsers/chromium.js:39: exports.getDriver = function(browserBinary, devenvPathAbsolute) On 2018/09/01 12:26:34, Sebastian Noack wrote: > Nit: I don't think that the path is absolute is an important enough detail to > bloat the argument name. I'd just call it "devenvPath". Done.
LGTM! 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
