Issue 29367181: Issue 4726 - Add tests for the element hiding emulation content script (Closed)
|
Created:
Dec. 11, 2016, 11:44 a.m. by Felix Dahlke Modified:
Jan. 19, 2017, 2:08 p.m. Reviewers:
kzar CC:
Wladimir Palant, Sebastian Noack Base URL:
https://bitbucket.org/fhd/adblockpluscore Visibility:
Public. |
DescriptionIssue 4726 - Add tests for the element hiding emulation content script
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Rebased on master, added tests for braces in regexp property selectors #
Total comments: 51
Patch Set 3 : Run browser tests via our test runner #Patch Set 4 : Add license header #
Total comments: 11
Patch Set 5 : Addressed remaining comments #
Total comments: 8
Patch Set 6 : Wrap and indent QUnit.test invocations consistently #
MessagesTotal messages: 19
https://codereview.adblockplus.org/29367181/diff/29367222/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367222/test/browser/elemHi... test/browser/elemHideEmulation.js:106: function() Not so sure about this indentation style - suggestions welcome.
Rebased, new patch set is up.
Cool idea, I like how the tests will still run from the command line with PhantomJS. I've had a quick look and left some comments. Will go through everything in more detail when you've addressed those. https://codereview.adblockplus.org/29367181/diff/29367382/README.md File README.md (right): https://codereview.adblockplus.org/29367181/diff/29367382/README.md#newcode12 README.md:12: ### Requirements If switching to this syntax for the headers mind updating the existing headers for consistency? https://codereview.adblockplus.org/29367181/diff/29367382/README.md#newcode23 README.md:23: ### Node.js tests I'd prefer to call these unit tests rather than Node.js tests. https://codereview.adblockplus.org/29367181/diff/29367382/package.json File package.json (right): https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode7 package.json:7: "node-qunit-phantomjs": "^1.5.0", I wonder what node-qunit-phantomjs really adds? Couldn't we have the test-browser command run the command we need directly instead and thus avoid the dependency? https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode9 package.json:9: "qunitjs": "^2.1.0", IMO seems kind of a hack to use a nodejs script from a HTML page, since the same APIs are not necessarily available. Maybe it's better to just include qunit.js in the browser tests directly ourselves? https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode13 package.json:13: "test": "npm run test-node; npm run test-browser", IMO we should instead modify test_runner.js. It would be cool if `node test` ran that and that in turn ran all the tests. (That script also accepts the paths of specific test files to run, that means you can run only a subset of the tests if you like. I think it should be updated to work when passed the paths to your browser tests too. That way you could tell the script to run one browser test and one node test file for example, that might be pretty useful if they were related.) https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) Can we not use more modern JavaScript features with PhantomJS? How about `for (let styleElement of styleElements)`? https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:29: QUnit.assert.hidden = function(element) I wonder if the URL polyfill and these utility function should be ain a separate helpers file? Sounds like they would be useful for a lot of tests. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:51: var styleElement; Nit: `let` rather than `var` throughout? https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; Nit: Since the else clause has braces the if clause should have them too. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:76: function(callback) Nit: Mind using arrow functions here and for the other anonymous functions? https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], Nit: This indentation looks weird. How about something like this? (using arrow functions whilst at it) applyElemHideEmulation( "[-abp-properties='background-color: rgb(0, 0, 0)']", () => { assert.hidden(toHide); done(); } );
Added patch sets 3 and 4 - and a whole lot of responses :) https://codereview.adblockplus.org/29367181/diff/29367382/README.md File README.md (right): https://codereview.adblockplus.org/29367181/diff/29367382/README.md#newcode12 README.md:12: ### Requirements On 2017/01/09 08:28:42, kzar wrote: > If switching to this syntax for the headers mind updating the existing headers > for consistency? Markdown only has the underlined syntax for level one and level two headers, hence level three headers need to use the prefixed syntax. I personally much prefer to mix them like this rather than to switch everything to prefixed. We do it that way in lots (if not most) of our READMEs. https://codereview.adblockplus.org/29367181/diff/29367382/README.md#newcode23 README.md:23: ### Node.js tests On 2017/01/09 08:28:42, kzar wrote: > I'd prefer to call these unit tests rather than Node.js tests. I'll rewrite this section when removing the `test-node` and `test-browser` scripts (see https://codereview.adblockplus.org/29367181/diff/29367382/package.json?contex...). https://codereview.adblockplus.org/29367181/diff/29367382/package.json File package.json (right): https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode7 package.json:7: "node-qunit-phantomjs": "^1.5.0", On 2017/01/09 08:28:43, kzar wrote: > I wonder what node-qunit-phantomjs really adds? Couldn't we have the > test-browser command run the command we need directly instead and thus avoid the > dependency? QUnit tests run in an HTML page, we have to extract the results from there. In my understanding that's what node-qunit-phantomjs does. I would rather not have us do that ourselves. https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode9 package.json:9: "qunitjs": "^2.1.0", On 2017/01/09 08:28:42, kzar wrote: > IMO seems kind of a hack to use a nodejs script from a HTML page, since the same > APIs are not necessarily available. Maybe it's better to just include qunit.js > in the browser tests directly ourselves? qunitjs is a pure frontend dependency in my understanding - how would one use it without a browser? I don't see a problem with using NPM to download frontend dependencies if we're using it anyway. https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode13 package.json:13: "test": "npm run test-node; npm run test-browser", On 2017/01/09 08:28:43, kzar wrote: > IMO we should instead modify test_runner.js. It would be cool if `node test` ran > that and that in turn ran all the tests. > > (That script also accepts the paths of specific test files to run, that means > you can run only a subset of the tests if you like. I think it should be updated > to work when passed the paths to your browser tests too. That way you could tell > the script to run one browser test and one node test file for example, that > might be pretty useful if they were related.) Done. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/09 08:28:43, kzar wrote: > Can we not use more modern JavaScript features with PhantomJS? How about `for > (let styleElement of styleElements)`? Unfortunately not with version 2.1.7, that one still uses a version of WebKit that doesn't support let (or any other ES6 features we would want to use I think). The next release will presumably support this, then we can modernise (probably also the element hiding emulation content script itself). https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:29: QUnit.assert.hidden = function(element) On 2017/01/09 08:28:43, kzar wrote: > I wonder if the URL polyfill and these utility function should be ain a separate > helpers file? Sounds like they would be useful for a lot of tests. I personally prefer to share code once it needs to be shared, not before. So I would rather keep them here until there is a second test that needs them. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:51: var styleElement; On 2017/01/09 08:28:43, kzar wrote: > Nit: `let` rather than `var` throughout? See above. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/09 08:28:43, kzar wrote: > Nit: Since the else clause has braces the if clause should have them too. Why? I personally prefer it this way. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:76: function(callback) On 2017/01/09 08:28:43, kzar wrote: > Nit: Mind using arrow functions here and for the other anonymous functions? See above. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/09 08:28:43, kzar wrote: > Nit: This indentation looks weird. How about something like this? (using arrow > functions whilst at it) > > applyElemHideEmulation( > "[-abp-properties='background-color: rgb(0, 0, 0)']", > () => > { > assert.hidden(toHide); > done(); > } > ); Yeah I know, I did some research in our code base and this seems to be the way we normally do this. That way we don't need to indent the function body. I would be fine with breaking the first parameter as well, so we end up with: foo( [...], function() { }); I'm mostly concerned about consistency, so if we use a style like the following elsewhere, I would be OK with going for that: foo( [...], function() { }); Or even: foo( [...], function() { } ); Quite annoying edge case, really :D
https://codereview.adblockplus.org/29367181/diff/29367382/README.md File README.md (right): https://codereview.adblockplus.org/29367181/diff/29367382/README.md#newcode12 README.md:12: ### Requirements On 2017/01/10 09:16:47, Felix Dahlke wrote: > On 2017/01/09 08:28:42, kzar wrote: > > If switching to this syntax for the headers mind updating the existing headers > > for consistency? > > Markdown only has the underlined syntax for level one and level two headers, > hence level three headers need to use the prefixed syntax. I personally much > prefer to mix them like this rather than to switch everything to prefixed. We do > it that way in lots (if not most) of our READMEs. Well you're right the need for the different levels of headers means you need the prefixed syntax but I don't really see the point keeping the first two headers using the other syntax now that you're using it. Doing that looks inconsistent and therefore kind of ugly to me. That said I don't care enough to argue any more so I'll leave it up to you. https://codereview.adblockplus.org/29367181/diff/29367382/package.json File package.json (right): https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode7 package.json:7: "node-qunit-phantomjs": "^1.5.0", On 2017/01/10 09:16:47, Felix Dahlke wrote: > QUnit tests run in an HTML page, we have to extract the results from there. In > my understanding that's what node-qunit-phantomjs does. I would rather not have > us do that ourselves. Well it seems that most of the work is actually done by qunit-phantomjs-runner which provides the test results as JSON. It seems that all qunit-phantomjs-runner does is parse the JSON results and output them prettily. (The other thing it seems to do is have the phantomjs-prebuilt dependency which means phantomjs is installed automatically.) I wonder if we could add the phantomjs-prebuilt and qunit-phantomjs-runner dependencies ourselves instead. While at it it would also be nice if we could combine the results from both unit and browser tests, so that the test totals etc are correct. (It seems like at the moment we simply print the formatted results for both, one after another.) If we could get nodeunit to give us the results formatted as JSON too perhaps we could then figure out a way to have them all rendered by nodeunit together properly? (That would have the bonus of allowing us to print all the results in a way that a CI server understands in the future too.) https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode9 package.json:9: "qunitjs": "^2.1.0", On 2017/01/10 09:16:48, Felix Dahlke wrote: > On 2017/01/09 08:28:42, kzar wrote: > > IMO seems kind of a hack to use a nodejs script from a HTML page, since the > same > > APIs are not necessarily available. Maybe it's better to just include qunit.js > > in the browser tests directly ourselves? > > qunitjs is a pure frontend dependency in my understanding - how would one use it > without a browser? I don't see a problem with using NPM to download frontend > dependencies if we're using it anyway. Fair enough. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/10 09:16:49, Felix Dahlke wrote: > On 2017/01/09 08:28:43, kzar wrote: > > Can we not use more modern JavaScript features with PhantomJS? How about `for > > (let styleElement of styleElements)`? > > Unfortunately not with version 2.1.7, that one still uses a version of WebKit > that doesn't support let (or any other ES6 features we would want to use I > think). The next release will presumably support this, then we can modernise > (probably also the element hiding emulation content script itself). Damn, this could be a problem in the near future since we plan to use some of these features since Chrome 49 is otherwise the lowest common denominator. I guess we need to use babel or similar to transpile the scripts when they're run from PhantomJS for now until the next version comes out. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:29: QUnit.assert.hidden = function(element) On 2017/01/10 09:16:49, Felix Dahlke wrote: > On 2017/01/09 08:28:43, kzar wrote: > > I wonder if the URL polyfill and these utility function should be ain a > separate > > helpers file? Sounds like they would be useful for a lot of tests. > > I personally prefer to share code once it needs to be shared, not before. So I > would rather keep them here until there is a second test that needs them. Fair enough. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/10 09:16:48, Felix Dahlke wrote: > On 2017/01/09 08:28:43, kzar wrote: > > Nit: Since the else clause has braces the if clause should have them too. > > Why? I personally prefer it this way. I've been told to do that on codereviews before by Wladimir IIRC. I kind of see his point too, it's a little less clear otherwise. (If a bit more concise.) https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/10 09:16:49, Felix Dahlke wrote: > On 2017/01/09 08:28:43, kzar wrote: > > Nit: This indentation looks weird. How about something like this? (using arrow > > functions whilst at it) > > > > applyElemHideEmulation( > > "[-abp-properties='background-color: rgb(0, 0, 0)']", > > () => > > { > > assert.hidden(toHide); > > done(); > > } > > ); > > Yeah I know, I did some research in our code base and this seems to be the way > we normally do this. That way we don't need to indent the function body. I would > be fine with breaking the first parameter as well, so we end up with: > > foo( > [...], > function() > { > }); > > I'm mostly concerned about consistency, so if we use a style like the following > elsewhere, I would be OK with going for that: > > foo( > [...], > function() > { > }); > > Or even: > > foo( > [...], > function() > { > } > ); > > Quite annoying edge case, really :D Well I'm pretty sure we've done it the way I've suggested elsewhere, that's how I would do it and how I would expect it to be done. (I don't much feel like searching through the platform / core code for examples though.) https://codereview.adblockplus.org/29367181/diff/29370964/README.md File README.md (right): https://codereview.adblockplus.org/29367181/diff/29370964/README.md#newcode26 README.md:26: `npm test test/synchronizer.js`. Mind also giving an example that runs both a unit test and a browser test file? Just to make it extra clear for people new to the tests. https://codereview.adblockplus.org/29367181/diff/29370964/README.md#newcode32 README.md:32: opening `test/browser/index.html`. Since the browser tests don't have a build step I guess we could make this a link to the file. That way people could click on it to try the tests right away in their browser from the README. (When it's rendered in GitHub at least.) https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js#newc... test_runner.js:55: else Nit: Since the addTestPaths call goes over multiple lines we should add the braces to the else clause. Since we're adding braces to the else clause we should add it to the if clause too. https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js#newc... test_runner.js:58: true); Nit: We would normally put the closing parenthesis on the next line in line with addTestsPaths here. https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js#newc... test_runner.js:62: qunitFiles.forEach(file => qunit(path.resolve(file))); Nit: We usually use "for of" loops instead of forEach where possible.
https://codereview.adblockplus.org/29367181/diff/29367382/package.json File package.json (right): https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode7 package.json:7: "node-qunit-phantomjs": "^1.5.0", On 2017/01/10 11:05:33, kzar wrote: > On 2017/01/10 09:16:47, Felix Dahlke wrote: > > QUnit tests run in an HTML page, we have to extract the results from there. In > > my understanding that's what node-qunit-phantomjs does. I would rather not > have > > us do that ourselves. > > Well it seems that most of the work is actually done by qunit-phantomjs-runner > which provides the test results as JSON. It seems that all > qunit-phantomjs-runner does is parse the JSON results and output them prettily. > (The other thing it seems to do is have the phantomjs-prebuilt dependency which > means phantomjs is installed automatically.) > > I wonder if we could add the phantomjs-prebuilt and qunit-phantomjs-runner > dependencies ourselves instead. I guess we can, but given how qunit-phantomjs-runner and node-qunit-phantomjs are from the same author and thus about equally likely to become a problem for us - I don't see the point. > While at it it would also be nice if we could combine the results from both unit > and browser tests, so that the test totals etc are correct. (It seems like at > the moment we simply print the formatted results for both, one after another.) > If we could get nodeunit to give us the results formatted as JSON too perhaps we > could then figure out a way to have them all rendered by nodeunit together > properly? > > (That would have the bonus of allowing us to print all the results in a way that > a CI server understands in the future too.) Good point. I did a little bit of research and it looks like a bit of work, but we should be able to have both runners output the same standardised format. Seems like something we should tackle in a followup however - until we have proper CI that would be purely cosmetic, and this review is blocking important stuff. If you agree I'll create an issue. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/10 11:05:33, kzar wrote: > On 2017/01/10 09:16:48, Felix Dahlke wrote: > > On 2017/01/09 08:28:43, kzar wrote: > > > Nit: Since the else clause has braces the if clause should have them too. > > > > Why? I personally prefer it this way. > > I've been told to do that on codereviews before by Wladimir IIRC. I kind of see > his point too, it's a little less clear otherwise. (If a bit more concise.) IIRC he actually prefers it that way, but since we don't have a rule about that I would prefer to leave it as is. Doing things a certain way based on speculation about other people's personal preferences is not productive IMO. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/10 11:05:33, kzar wrote: > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > On 2017/01/09 08:28:43, kzar wrote: > > > Nit: This indentation looks weird. How about something like this? (using > arrow > > > functions whilst at it) > > > > > > applyElemHideEmulation( > > > "[-abp-properties='background-color: rgb(0, 0, 0)']", > > > () => > > > { > > > assert.hidden(toHide); > > > done(); > > > } > > > ); > > > > Yeah I know, I did some research in our code base and this seems to be the way > > we normally do this. That way we don't need to indent the function body. I > would > > be fine with breaking the first parameter as well, so we end up with: > > > > foo( > > [...], > > function() > > { > > }); > > > > I'm mostly concerned about consistency, so if we use a style like the > following > > elsewhere, I would be OK with going for that: > > > > foo( > > [...], > > function() > > { > > }); > > > > Or even: > > > > foo( > > [...], > > function() > > { > > } > > ); > > > > Quite annoying edge case, really :D > > Well I'm pretty sure we've done it the way I've suggested elsewhere, that's how > I would do it and how I would expect it to be done. (I don't much feel like > searching through the platform / core code for examples though.) Then I figure we should leave it as-is for the sake of progress. I think we should just find a linter and configure it so that everybody is happy, then we will have everything consistent.
https://codereview.adblockplus.org/29367181/diff/29367382/package.json File package.json (right): https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode7 package.json:7: "node-qunit-phantomjs": "^1.5.0", On 2017/01/10 13:27:32, Felix Dahlke wrote: > On 2017/01/10 11:05:33, kzar wrote: > > On 2017/01/10 09:16:47, Felix Dahlke wrote: > > > QUnit tests run in an HTML page, we have to extract the results from there. > In > > > my understanding that's what node-qunit-phantomjs does. I would rather not > > have > > > us do that ourselves. > > > > Well it seems that most of the work is actually done by qunit-phantomjs-runner > > which provides the test results as JSON. It seems that all > > qunit-phantomjs-runner does is parse the JSON results and output them > prettily. > > (The other thing it seems to do is have the phantomjs-prebuilt dependency > which > > means phantomjs is installed automatically.) > > > > I wonder if we could add the phantomjs-prebuilt and qunit-phantomjs-runner > > dependencies ourselves instead. > > I guess we can, but given how qunit-phantomjs-runner and node-qunit-phantomjs > are from the same author and thus about equally likely to become a problem for > us - I don't see the point. Well the point is to minimise dependencies. One package depends on the other + more. The fewer dependencies the better right? That's certainly how I remember my reviews being treated, for example the Crowdin translation sync where we went to painful lengths to replace the requests library with urllib3 on which it depended. > > > While at it it would also be nice if we could combine the results from both > unit > > and browser tests, so that the test totals etc are correct. (It seems like at > > the moment we simply print the formatted results for both, one after another.) > > If we could get nodeunit to give us the results formatted as JSON too perhaps > we > > could then figure out a way to have them all rendered by nodeunit together > > properly? > > > > (That would have the bonus of allowing us to print all the results in a way > that > > a CI server understands in the future too.) > > Good point. I did a little bit of research and it looks like a bit of work, but > we should be able to have both runners output the same standardised format. > Seems like something we should tackle in a followup however - until we have > proper CI that would be purely cosmetic, and this review is blocking important > stuff. If you agree I'll create an issue. Wouldn't it be better to just do it properly now? (I don't think this blocks the other work since you can still run the tests even if they're not passed review and pushed.) FWIW I agree that the output for CI can be tackled as a future issue since we don't have any CI yet anyway. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/10 11:05:33, kzar wrote: > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > On 2017/01/09 08:28:43, kzar wrote: > > > Can we not use more modern JavaScript features with PhantomJS? How about > `for > > > (let styleElement of styleElements)`? > > > > Unfortunately not with version 2.1.7, that one still uses a version of WebKit > > that doesn't support let (or any other ES6 features we would want to use I > > think). The next release will presumably support this, then we can modernise > > (probably also the element hiding emulation content script itself). > > Damn, this could be a problem in the near future since we plan to use some of > these features since Chrome 49 is otherwise the lowest common denominator. I > guess we need to use babel or similar to transpile the scripts when they're run > from PhantomJS for now until the next version comes out. You missed this one? (Some other smaller ones in the other files too.) https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/10 13:27:32, Felix Dahlke wrote: > IIRC he actually prefers it that way, but since we don't have a rule > about that Doing things a certain way based on speculation about other > people's personal preferences is not productive IMO. OK well how's this? I have been asked to use consistent braces in the past and have come to prefer them too. Mind adding them here? https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/10 13:27:32, Felix Dahlke wrote: > Then I figure we should leave it as-is for the sake of progress. I > think we should just find a linter and configure it so that everybody > is happy, then we will have everything consistent. Or alternatively you could just improve the indentation here for the sake of progress? How it stands is ugly IMO and it sounds like you're not too keen on how it looks either from your comment.
No new patch sets at this point although there are comments which I want to address - mainly because it's not really worth more effort if we cannot agree on the open questions. https://codereview.adblockplus.org/29367181/diff/29367382/package.json File package.json (right): https://codereview.adblockplus.org/29367181/diff/29367382/package.json#newcode7 package.json:7: "node-qunit-phantomjs": "^1.5.0", On 2017/01/11 04:25:28, kzar wrote: > On 2017/01/10 13:27:32, Felix Dahlke wrote: > > On 2017/01/10 11:05:33, kzar wrote: > > > On 2017/01/10 09:16:47, Felix Dahlke wrote: > > > > QUnit tests run in an HTML page, we have to extract the results from > there. > > In > > > > my understanding that's what node-qunit-phantomjs does. I would rather not > > > have > > > > us do that ourselves. > > > > > > Well it seems that most of the work is actually done by > qunit-phantomjs-runner > > > which provides the test results as JSON. It seems that all > > > qunit-phantomjs-runner does is parse the JSON results and output them > > prettily. > > > (The other thing it seems to do is have the phantomjs-prebuilt dependency > > which > > > means phantomjs is installed automatically.) > > > > > > I wonder if we could add the phantomjs-prebuilt and qunit-phantomjs-runner > > > dependencies ourselves instead. > > > > I guess we can, but given how qunit-phantomjs-runner and node-qunit-phantomjs > > are from the same author and thus about equally likely to become a problem for > > us - I don't see the point. > > Well the point is to minimise dependencies. One package depends on the other + > more. The fewer dependencies the better right? That's certainly how I remember > my reviews being treated, for example the Crowdin translation sync where we went > to painful lengths to replace the requests library with urllib3 on which it > depended. It's not that simple. We should aim to minimise dependencies in general, but not religiously, it really depends on the case. In the case of the requests library vs urllib3, my understanding is that the former is a somewhat "unnecessary" wrapper around urllib3. I don't know the details so I don't know if that is true and if not using the request library was a wise decision. I see four problems with dependencies: 1) The dependency could cease to be maintained, in which case we would have to fork or replace it 2) The dependency increases complexity by handling more use cases than just ours 3) The dependency would be hard to update/replace if we want/need to 4) The dependency could violate someone else's copyright, a problem we would inherit by using it under an open source license. I personally have run into traps #1, #2 and #3 countless times, #4 once. Wladimir had similar experiences, which lead us to be careful with external dependencies from the start. At the same time, we need to find the right balance between these dangers and pragmatism (i.e. how much time/trouble it saves us to use a dependency vs what the actual risks with it are). In this particular case, #1 does not really apply, since it was written by the very same author to be used in tandem. In fact, it seems more like he split one package up into two for modularisation purposes and nothing else. #2 does from what I see not really apply either, since node-qunit-phantomjs does exactly one thing - support the use case we need. #3 is actually _better_ if we use this, since we currently only make a single API call, whereas if we would use qunit-phantomjs-runner directly we would use more of its API. Therefore I see absolutely no reason not to use node-qunit-phantomjs right now. If we do end up parsing test results ourselves however, we probably want/need to get rid of it. > > > > > While at it it would also be nice if we could combine the results from both > > unit > > > and browser tests, so that the test totals etc are correct. (It seems like > at > > > the moment we simply print the formatted results for both, one after > another.) > > > If we could get nodeunit to give us the results formatted as JSON too > perhaps > > we > > > could then figure out a way to have them all rendered by nodeunit together > > > properly? > > > > > > (That would have the bonus of allowing us to print all the results in a way > > that > > > a CI server understands in the future too.) > > > > Good point. I did a little bit of research and it looks like a bit of work, > but > > we should be able to have both runners output the same standardised format. > > Seems like something we should tackle in a followup however - until we have > > proper CI that would be purely cosmetic, and this review is blocking important > > stuff. If you agree I'll create an issue. > > Wouldn't it be better to just do it properly now? (I don't think this blocks the > other work since you can still run the tests even if they're not passed review > and pushed.) FWIW I agree that the output for CI can be tackled as a future > issue since we don't have any CI yet anyway. That's exactly the part I mean. I feel pretty strong about wanting to use node-qunit-phantomjs as long as we don't work on having the same output format, see the reasoning above. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/11 04:25:28, kzar wrote: > On 2017/01/10 11:05:33, kzar wrote: > > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > > On 2017/01/09 08:28:43, kzar wrote: > > > > Can we not use more modern JavaScript features with PhantomJS? How about > > `for > > > > (let styleElement of styleElements)`? > > > > > > Unfortunately not with version 2.1.7, that one still uses a version of > WebKit > > > that doesn't support let (or any other ES6 features we would want to use I > > > think). The next release will presumably support this, then we can modernise > > > (probably also the element hiding emulation content script itself). > > > > Damn, this could be a problem in the near future since we plan to use some of > > these features since Chrome 49 is otherwise the lowest common denominator. I > > guess we need to use babel or similar to transpile the scripts when they're > run > > from PhantomJS for now until the next version comes out. > > You missed this one? (Some other smaller ones in the other files too.) No, it rather sounded like you thought this could become a problem in the future - which it could. Right now, elemHideEmulation.js is still ES5, and will most likely continue to be at least until :has support has landed. By that time, PhantomJS should have made a release (they always seem to release in January). I don't think being able to use some ES6 features a few weeks earlier is worth the extra hassle of introducing babel here. We could create an issue for this, so we won't forget about it. What do you think? https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/11 04:25:29, kzar wrote: > On 2017/01/10 13:27:32, Felix Dahlke wrote: > > IIRC he actually prefers it that way, but since we don't have a rule > > about that Doing things a certain way based on speculation about other > > people's personal preferences is not productive IMO. > > OK well how's this? I have been asked to use consistent braces in the past and > have come to prefer them too. Mind adding them here? What others have asked in other places is not particularly relevant: We have deliberately not standarised braces around single statement bodies to leave it up to the people working on a module/file/function to decide. We might want to revisit that in the future if it leads to wasteful arguments, but right now all that really counts is what YOU find more readable. Since you indicated you do find it more readable I will add them :) https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/11 04:25:29, kzar wrote: > On 2017/01/10 13:27:32, Felix Dahlke wrote: > > Then I figure we should leave it as-is for the sake of progress. I > > think we should just find a linter and configure it so that everybody > > is happy, then we will have everything consistent. > > Or alternatively you could just improve the indentation here for the sake of > progress? How it stands is ugly IMO and it sounds like you're not too keen on > how it looks either from your comment. I don't find it very beautiful, but it does look like the best option to me. Back when I wrote this, I went to look for examples of how we generally do this. IIRC I couldn't find any examples (much of the code in core doesn't comply with the 80 column rule yet), so I looked at other JavaScript projects, and this style (with the `{` on the same line of course) was pretty prominent. Why I like it: This way the way the argument list is being wrapped does not affect the indentation of the function body. In this case it's pretty trivial, but if you look at the last test case below, the entire test case code is indented by one level solely based on low long the test summary is. I see two other options that keep the indentation of the function body stable: 1. We turn the anonymous function into a named function that is declared right before it is being used. 2. We deliberately violate the 80 column rule by not breaking, which can be done when adhering to it would hurt readability. My preferred approach is what's up right now. If you would rather not have that, I would prefer #1. If you don't like that either, #2. Indenting the function body is something I'd like to avoid if possible - at least for the test cases below, in this case I would be OK with it. What do you think?
https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/11 10:06:02, Felix Dahlke wrote: > On 2017/01/11 04:25:28, kzar wrote: > > On 2017/01/10 11:05:33, kzar wrote: > > > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > > > On 2017/01/09 08:28:43, kzar wrote: > > > > > Can we not use more modern JavaScript features with PhantomJS? How about > > > `for > > > > > (let styleElement of styleElements)`? > > > > > > > > Unfortunately not with version 2.1.7, that one still uses a version of > > WebKit > > > > that doesn't support let (or any other ES6 features we would want to use I > > > > think). The next release will presumably support this, then we can > modernise > > > > (probably also the element hiding emulation content script itself). > > > > > > Damn, this could be a problem in the near future since we plan to use some > of > > > these features since Chrome 49 is otherwise the lowest common denominator. I > > > guess we need to use babel or similar to transpile the scripts when they're > > run > > > from PhantomJS for now until the next version comes out. > > > > You missed this one? (Some other smaller ones in the other files too.) > > No, it rather sounded like you thought this could become a problem in the future > - which it could. Right now, elemHideEmulation.js is still ES5, and will most > likely continue to be at least until :has support has landed. By that time, > PhantomJS should have made a release (they always seem to release in January). I > don't think being able to use some ES6 features a few weeks earlier is worth the > extra hassle of introducing babel here. We could create an issue for this, so we > won't forget about it. What do you think? Please at least put a comment / note in relevant places warning that the file(s) only supports ES5 due to PhantomJS support. (Mention the version of PhantomJS needed for us to improve things.) We're looking to move code over to using `let`, arrow functions etc so we need to avoid breaking these tests by mistake. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/11 10:06:02, Felix Dahlke wrote: > On 2017/01/11 04:25:29, kzar wrote: > > On 2017/01/10 13:27:32, Felix Dahlke wrote: > > > IIRC he actually prefers it that way, but since we don't have a rule > > > about that Doing things a certain way based on speculation about other > > > people's personal preferences is not productive IMO. > > > > OK well how's this? I have been asked to use consistent braces in the past and > > have come to prefer them too. Mind adding them here? > > What others have asked in other places is not particularly relevant: We have > deliberately not standarised braces around single statement bodies to leave it > up to the people working on a module/file/function to decide. We might want to > revisit that in the future if it leads to wasteful arguments, but right now all > that really counts is what YOU find more readable. Since you indicated you do > find it more readable I will add them :) I think it is relevant what Wladimir has told me in the past since he's the module owner and I'm just reviewing this for him since he's away. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/11 10:06:02, Felix Dahlke wrote: > Indenting the function body is something I'd like to avoid if possible > - at least for the test cases below, in this case I would be OK with > it. Well we "indent the function body" all the time, for example above in this very file. By your logic shouldn't elemHideEmulation.load(function() { elemHideEmulation.apply(); callback(); }); become this? elemHideEmulation.load(function() { elemHideEmulation.apply(); callback(); }); I don't think I've ever see the opening bracket for a function definition indented how you're suggesting, or at least I can't remember when I have. Perhaps like you say it's a convention in other projects, I don't know. > What do you think? Well I think this applyElemHideEmulation call should look like this: applyElemHideEmulation( ["[-abp-properties='background-color: rgb(0, 0, 0)']"], function() { assert.hidden(toHide); done(); }) ); and if you're worried about the indentation being consistent between all the tests you can wrap the ones with shorter descriptions more eagerly than is strictly required, no harm in that.
https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/11 11:19:18, kzar wrote: > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > On 2017/01/11 04:25:28, kzar wrote: > > > On 2017/01/10 11:05:33, kzar wrote: > > > > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > > > > On 2017/01/09 08:28:43, kzar wrote: > > > > > > Can we not use more modern JavaScript features with PhantomJS? How > about > > > > `for > > > > > > (let styleElement of styleElements)`? > > > > > > > > > > Unfortunately not with version 2.1.7, that one still uses a version of > > > WebKit > > > > > that doesn't support let (or any other ES6 features we would want to use > I > > > > > think). The next release will presumably support this, then we can > > modernise > > > > > (probably also the element hiding emulation content script itself). > > > > > > > > Damn, this could be a problem in the near future since we plan to use some > > of > > > > these features since Chrome 49 is otherwise the lowest common denominator. > I > > > > guess we need to use babel or similar to transpile the scripts when > they're > > > run > > > > from PhantomJS for now until the next version comes out. > > > > > > You missed this one? (Some other smaller ones in the other files too.) > > > > No, it rather sounded like you thought this could become a problem in the > future > > - which it could. Right now, elemHideEmulation.js is still ES5, and will most > > likely continue to be at least until :has support has landed. By that time, > > PhantomJS should have made a release (they always seem to release in January). > I > > don't think being able to use some ES6 features a few weeks earlier is worth > the > > extra hassle of introducing babel here. We could create an issue for this, so > we > > won't forget about it. What do you think? > > Please at least put a comment / note in relevant places warning that the file(s) > only supports ES5 due to PhantomJS support. (Mention the version of PhantomJS > needed for us to improve things.) We're looking to move code over to using > `let`, arrow functions etc so we need to avoid breaking these tests by mistake. Sure! This affects chrome/content/elemHideEmulation.js, test/browser/elemHideEmulation.js and also lib/common.js. I will add a note to all three files and refer to an issue I'll create about being unable to use ES5 features in some files because of the PhantomJS version we use. Only one new file is being added here, the others were ES5 before. However, to my knowledge, there is nothing holding us back from using selected ES6 features in these files except for PhantomJS. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/11 11:19:18, kzar wrote: > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > Indenting the function body is something I'd like to avoid if possible > > - at least for the test cases below, in this case I would be OK with > > it. > > Well we "indent the function body" all the time, for example above in this very > file. By your logic shouldn't > > elemHideEmulation.load(function() > { > elemHideEmulation.apply(); > callback(); > }); > > become this? > > elemHideEmulation.load(function() > { > elemHideEmulation.apply(); > callback(); > }); No, I mean the opening brace of an anonymous function passed as an argument is on the same indentation level as the function invocation. My qualm with all this is this edge case: QUnit.test("This and that works properly", function(assert) { [long test body] }); If we would just decide that the description is not elaborate enough, changing just a single string will have us reindent the entire body: QUnit.test("This and that works properly unless another things happens", function(assert) { [long test body] } ); We do not have this issues with the three alternatives I offered: A. We indent the opening brace of an anonymous function passed as an argument at the same level as the function invocation (my preferred approach, seems most popular in other JS projects that have a max column rule). B. We create a named function that is then passed to the function invocation. C. We deliberately violate the 80 column rule in these cases (that is how we deal with this in the existing code base, since huge chunks of it still don't comply with the 80 column rule). > Well I think this applyElemHideEmulation call should look like this: > > applyElemHideEmulation( > ["[-abp-properties='background-color: rgb(0, 0, 0)']"], > function() > { > assert.hidden(toHide); > done(); > }) > ); For small functions such as this, I really wouldn't mind this approach. Just for long functions do I find it problematic, it'll cause a high number of unrelated changes if we change a single argument in a function invocation, mess up hg blame etc. > and if you're worried about the indentation being consistent between all the > tests you can wrap the ones with shorter descriptions more eagerly than is > strictly required, no harm in that. So all test case bodies would always have to be indented one level further? That would avoid the diff/blame issues I went into above, but I think it is a rule that will be hard to follow consistently - it just doesn't make sense when none of your QUnit.test invocations would actually violate the 80 column rule. I'm fine with any of A, B or C here. I find A to be the most elegant solution. Sure, it would be inconsistent with how we do it for small functions, but sometimes consistency can be sacrificed for readability. What do you think? As a last resort I could try to keep every test description below a certain threshold to avoid this situation altogether. Not a fantastic solution, but it'd do the trick...
Mind commenting on the indentation discussion in test/browser/elemHideEmulation.js line 105 Sebastian? I don't have much more to say there. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/12 09:24:06, Felix Dahlke wrote: > On 2017/01/11 11:19:18, kzar wrote: > > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > > On 2017/01/11 04:25:28, kzar wrote: > > > > On 2017/01/10 11:05:33, kzar wrote: > > > > > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > > > > > On 2017/01/09 08:28:43, kzar wrote: > > > > > > > Can we not use more modern JavaScript features with PhantomJS? How > > about > > > > > `for > > > > > > > (let styleElement of styleElements)`? > > > > > > > > > > > > Unfortunately not with version 2.1.7, that one still uses a version of > > > > WebKit > > > > > > that doesn't support let (or any other ES6 features we would want to > use > > I > > > > > > think). The next release will presumably support this, then we can > > > modernise > > > > > > (probably also the element hiding emulation content script itself). > > > > > > > > > > Damn, this could be a problem in the near future since we plan to use > some > > > of > > > > > these features since Chrome 49 is otherwise the lowest common > denominator. > > I > > > > > guess we need to use babel or similar to transpile the scripts when > > they're > > > > run > > > > > from PhantomJS for now until the next version comes out. > > > > > > > > You missed this one? (Some other smaller ones in the other files too.) > > > > > > No, it rather sounded like you thought this could become a problem in the > > future > > > - which it could. Right now, elemHideEmulation.js is still ES5, and will > most > > > likely continue to be at least until :has support has landed. By that time, > > > PhantomJS should have made a release (they always seem to release in > January). > > I > > > don't think being able to use some ES6 features a few weeks earlier is worth > > the > > > extra hassle of introducing babel here. We could create an issue for this, > so > > we > > > won't forget about it. What do you think? > > > > Please at least put a comment / note in relevant places warning that the > file(s) > > only supports ES5 due to PhantomJS support. (Mention the version of PhantomJS > > needed for us to improve things.) We're looking to move code over to using > > `let`, arrow functions etc so we need to avoid breaking these tests by > mistake. > > Sure! This affects chrome/content/elemHideEmulation.js, > test/browser/elemHideEmulation.js and also lib/common.js. I will add a note to > all three files and refer to an issue I'll create about being unable to use ES5 > features in some files because of the PhantomJS version we use. Only one new > file is being added here, the others were ES5 before. However, to my knowledge, > there is nothing holding us back from using selected ES6 features in these files > except for PhantomJS. Cool, sounds good. (IMO an issue is overkill since there's a lot of other code in adblockplus{chrome,core,ui} that I'm/we're planning to migrate over anyway, but up to you.) https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/12 09:24:06, Felix Dahlke wrote: > On 2017/01/11 11:19:18, kzar wrote: > > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > > Indenting the function body is something I'd like to avoid if possible > > > - at least for the test cases below, in this case I would be OK with > > > it. > > > > Well we "indent the function body" all the time, for example above in this > very > > file. By your logic shouldn't > > > > elemHideEmulation.load(function() > > { > > elemHideEmulation.apply(); > > callback(); > > }); > > > > become this? > > > > elemHideEmulation.load(function() > > { > > elemHideEmulation.apply(); > > callback(); > > }); > > No, I mean the opening brace of an anonymous function passed as an argument is > on the same indentation level as the function invocation. > > My qualm with all this is this edge case: > > QUnit.test("This and that works properly", function(assert) > { > [long test body] > }); > > If we would just decide that the description is not elaborate enough, changing > just a single string will have us reindent the entire body: > > QUnit.test("This and that works properly unless another things happens", > function(assert) > { > [long test body] > } > ); > > We do not have this issues with the three alternatives I offered: > > A. We indent the opening brace of an anonymous function passed as an argument at > the same level as the function invocation (my preferred approach, seems most > popular in other JS projects that have a max column rule). > > B. We create a named function that is then passed to the function invocation. > > C. We deliberately violate the 80 column rule in these cases (that is how we > deal with this in the existing code base, since huge chunks of it still don't > comply with the 80 column rule). > > > > Well I think this applyElemHideEmulation call should look like this: > > > > applyElemHideEmulation( > > ["[-abp-properties='background-color: rgb(0, 0, 0)']"], > > function() > > { > > assert.hidden(toHide); > > done(); > > }) > > ); > > For small functions such as this, I really wouldn't mind this approach. Just for > long functions do I find it problematic, it'll cause a high number of unrelated > changes if we change a single argument in a function invocation, mess up hg > blame etc. > > > and if you're worried about the indentation being consistent between all the > > tests you can wrap the ones with shorter descriptions more eagerly than is > > strictly required, no harm in that. > > So all test case bodies would always have to be indented one level further? That > would avoid the diff/blame issues I went into above, but I think it is a rule > that will be hard to follow consistently - it just doesn't make sense when none > of your QUnit.test invocations would actually violate the 80 column rule. > > I'm fine with any of A, B or C here. I find A to be the most elegant solution. > Sure, it would be inconsistent with how we do it for small functions, but > sometimes consistency can be sacrificed for readability. What do you think? > > As a last resort I could try to keep every test description below a certain > threshold to avoid this situation altogether. Not a fantastic solution, but it'd > do the trick... I really think that you're overthinking this and should simply indent the functions like I suggest. Diff tools can handle a bit of whitespace changing.
https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/12 09:42:13, kzar wrote: > On 2017/01/12 09:24:06, Felix Dahlke wrote: > > On 2017/01/11 11:19:18, kzar wrote: > > > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > > > Indenting the function body is something I'd like to avoid if possible > > > > - at least for the test cases below, in this case I would be OK with > > > > it. > > > > > > Well we "indent the function body" all the time, for example above in this > > very > > > file. By your logic shouldn't > > > > > > elemHideEmulation.load(function() > > > { > > > elemHideEmulation.apply(); > > > callback(); > > > }); > > > > > > become this? > > > > > > elemHideEmulation.load(function() > > > { > > > elemHideEmulation.apply(); > > > callback(); > > > }); > > > > No, I mean the opening brace of an anonymous function passed as an argument is > > on the same indentation level as the function invocation. > > > > My qualm with all this is this edge case: > > > > QUnit.test("This and that works properly", function(assert) > > { > > [long test body] > > }); > > > > If we would just decide that the description is not elaborate enough, changing > > just a single string will have us reindent the entire body: > > > > QUnit.test("This and that works properly unless another things happens", > > function(assert) > > { > > [long test body] > > } > > ); > > > > We do not have this issues with the three alternatives I offered: > > > > A. We indent the opening brace of an anonymous function passed as an argument > at > > the same level as the function invocation (my preferred approach, seems most > > popular in other JS projects that have a max column rule). > > > > B. We create a named function that is then passed to the function invocation. > > > > C. We deliberately violate the 80 column rule in these cases (that is how we > > deal with this in the existing code base, since huge chunks of it still don't > > comply with the 80 column rule). > > > > > > > Well I think this applyElemHideEmulation call should look like this: > > > > > > applyElemHideEmulation( > > > ["[-abp-properties='background-color: rgb(0, 0, 0)']"], > > > function() > > > { > > > assert.hidden(toHide); > > > done(); > > > }) > > > ); > > > > For small functions such as this, I really wouldn't mind this approach. Just > for > > long functions do I find it problematic, it'll cause a high number of > unrelated > > changes if we change a single argument in a function invocation, mess up hg > > blame etc. > > > > > and if you're worried about the indentation being consistent between all the > > > tests you can wrap the ones with shorter descriptions more eagerly than is > > > strictly required, no harm in that. > > > > So all test case bodies would always have to be indented one level further? > That > > would avoid the diff/blame issues I went into above, but I think it is a rule > > that will be hard to follow consistently - it just doesn't make sense when > none > > of your QUnit.test invocations would actually violate the 80 column rule. > > > > I'm fine with any of A, B or C here. I find A to be the most elegant solution. > > Sure, it would be inconsistent with how we do it for small functions, but > > sometimes consistency can be sacrificed for readability. What do you think? > > > > As a last resort I could try to keep every test description below a certain > > threshold to avoid this situation altogether. Not a fantastic solution, but > it'd > > do the trick... > > I really think that you're overthinking this and should simply indent the > functions like I suggest. Diff tools can handle a bit of whitespace changing. I agree that putting the function header on a line with a different level of indentation than the braces that encapsulate the function body looks quite confusing. Neither do I recall having seen this in our code base before. Moreover, do I also find it inconsistent and difficult to parse if some arguments are given on the line which initiated the function call while some are wrapped in a way that it doesn't align with the initial arguments. As for consistency across our code base, here are two examples where we do it like Dave and I are suggesting: https://hg.adblockplus.org/adblockpluschrome/file/tip/chrome/devtools.js#l25 https://hg.adblockplus.org/adblockpluschrome/file/tip/composer.js#l41 I cannot see any code in this file where the addional indentation would cause exceeding the 80 characters limit, and if there is this would be an indication that you probably shouldn't use an anonymous function there in the first place.
Rename browser test runner, address code style comments
Address remaining comments
Patch set 5 is up, I think I've got everything now! https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:24: for (var i = 0; i < styleElements.length; i++) On 2017/01/12 09:42:13, kzar wrote: > On 2017/01/12 09:24:06, Felix Dahlke wrote: > > On 2017/01/11 11:19:18, kzar wrote: > > > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > > > On 2017/01/11 04:25:28, kzar wrote: > > > > > On 2017/01/10 11:05:33, kzar wrote: > > > > > > On 2017/01/10 09:16:49, Felix Dahlke wrote: > > > > > > > On 2017/01/09 08:28:43, kzar wrote: > > > > > > > > Can we not use more modern JavaScript features with PhantomJS? How > > > about > > > > > > `for > > > > > > > > (let styleElement of styleElements)`? > > > > > > > > > > > > > > Unfortunately not with version 2.1.7, that one still uses a version > of > > > > > WebKit > > > > > > > that doesn't support let (or any other ES6 features we would want to > > use > > > I > > > > > > > think). The next release will presumably support this, then we can > > > > modernise > > > > > > > (probably also the element hiding emulation content script itself). > > > > > > > > > > > > Damn, this could be a problem in the near future since we plan to use > > some > > > > of > > > > > > these features since Chrome 49 is otherwise the lowest common > > denominator. > > > I > > > > > > guess we need to use babel or similar to transpile the scripts when > > > they're > > > > > run > > > > > > from PhantomJS for now until the next version comes out. > > > > > > > > > > You missed this one? (Some other smaller ones in the other files too.) > > > > > > > > No, it rather sounded like you thought this could become a problem in the > > > future > > > > - which it could. Right now, elemHideEmulation.js is still ES5, and will > > most > > > > likely continue to be at least until :has support has landed. By that > time, > > > > PhantomJS should have made a release (they always seem to release in > > January). > > > I > > > > don't think being able to use some ES6 features a few weeks earlier is > worth > > > the > > > > extra hassle of introducing babel here. We could create an issue for this, > > so > > > we > > > > won't forget about it. What do you think? > > > > > > Please at least put a comment / note in relevant places warning that the > > file(s) > > > only supports ES5 due to PhantomJS support. (Mention the version of > PhantomJS > > > needed for us to improve things.) We're looking to move code over to using > > > `let`, arrow functions etc so we need to avoid breaking these tests by > > mistake. > > > > Sure! This affects chrome/content/elemHideEmulation.js, > > test/browser/elemHideEmulation.js and also lib/common.js. I will add a note to > > all three files and refer to an issue I'll create about being unable to use > ES5 > > features in some files because of the PhantomJS version we use. Only one new > > file is being added here, the others were ES5 before. However, to my > knowledge, > > there is nothing holding us back from using selected ES6 features in these > files > > except for PhantomJS. > > Cool, sounds good. (IMO an issue is overkill since there's a lot of other code > in adblockplus{chrome,core,ui} that I'm/we're planning to migrate over anyway, > but up to you.) Done. https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:54: styleElement = styleElements[0]; On 2017/01/11 11:19:19, kzar wrote: > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > On 2017/01/11 04:25:29, kzar wrote: > > > On 2017/01/10 13:27:32, Felix Dahlke wrote: > > > > IIRC he actually prefers it that way, but since we don't have a rule > > > > about that Doing things a certain way based on speculation about other > > > > people's personal preferences is not productive IMO. > > > > > > OK well how's this? I have been asked to use consistent braces in the past > and > > > have come to prefer them too. Mind adding them here? > > > > What others have asked in other places is not particularly relevant: We have > > deliberately not standarised braces around single statement bodies to leave it > > up to the people working on a module/file/function to decide. We might want to > > revisit that in the future if it leads to wasteful arguments, but right now > all > > that really counts is what YOU find more readable. Since you indicated you do > > find it more readable I will add them :) > > I think it is relevant what Wladimir has told me in the past since he's the > module owner and I'm just reviewing this for him since he's away. You're not reviewing it for him, you're reviewing it :) Anyway, done! https://codereview.adblockplus.org/29367181/diff/29367382/test/browser/elemHi... test/browser/elemHideEmulation.js:105: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/12 23:12:58, Sebastian Noack wrote: > On 2017/01/12 09:42:13, kzar wrote: > > On 2017/01/12 09:24:06, Felix Dahlke wrote: > > > On 2017/01/11 11:19:18, kzar wrote: > > > > On 2017/01/11 10:06:02, Felix Dahlke wrote: > > > > > Indenting the function body is something I'd like to avoid if possible > > > > > - at least for the test cases below, in this case I would be OK with > > > > > it. > > > > > > > > Well we "indent the function body" all the time, for example above in this > > > very > > > > file. By your logic shouldn't > > > > > > > > elemHideEmulation.load(function() > > > > { > > > > elemHideEmulation.apply(); > > > > callback(); > > > > }); > > > > > > > > become this? > > > > > > > > elemHideEmulation.load(function() > > > > { > > > > elemHideEmulation.apply(); > > > > callback(); > > > > }); > > > > > > No, I mean the opening brace of an anonymous function passed as an argument > is > > > on the same indentation level as the function invocation. > > > > > > My qualm with all this is this edge case: > > > > > > QUnit.test("This and that works properly", function(assert) > > > { > > > [long test body] > > > }); > > > > > > If we would just decide that the description is not elaborate enough, > changing > > > just a single string will have us reindent the entire body: > > > > > > QUnit.test("This and that works properly unless another things happens", > > > function(assert) > > > { > > > [long test body] > > > } > > > ); > > > > > > We do not have this issues with the three alternatives I offered: > > > > > > A. We indent the opening brace of an anonymous function passed as an > argument > > at > > > the same level as the function invocation (my preferred approach, seems most > > > popular in other JS projects that have a max column rule). > > > > > > B. We create a named function that is then passed to the function > invocation. > > > > > > C. We deliberately violate the 80 column rule in these cases (that is how we > > > deal with this in the existing code base, since huge chunks of it still > don't > > > comply with the 80 column rule). > > > > > > > > > > Well I think this applyElemHideEmulation call should look like this: > > > > > > > > applyElemHideEmulation( > > > > ["[-abp-properties='background-color: rgb(0, 0, 0)']"], > > > > function() > > > > { > > > > assert.hidden(toHide); > > > > done(); > > > > }) > > > > ); > > > > > > For small functions such as this, I really wouldn't mind this approach. Just > > for > > > long functions do I find it problematic, it'll cause a high number of > > unrelated > > > changes if we change a single argument in a function invocation, mess up hg > > > blame etc. > > > > > > > and if you're worried about the indentation being consistent between all > the > > > > tests you can wrap the ones with shorter descriptions more eagerly than is > > > > strictly required, no harm in that. > > > > > > So all test case bodies would always have to be indented one level further? > > That > > > would avoid the diff/blame issues I went into above, but I think it is a > rule > > > that will be hard to follow consistently - it just doesn't make sense when > > none > > > of your QUnit.test invocations would actually violate the 80 column rule. > > > > > > I'm fine with any of A, B or C here. I find A to be the most elegant > solution. > > > Sure, it would be inconsistent with how we do it for small functions, but > > > sometimes consistency can be sacrificed for readability. What do you think? > > > > > > As a last resort I could try to keep every test description below a certain > > > threshold to avoid this situation altogether. Not a fantastic solution, but > > it'd > > > do the trick... > > > > I really think that you're overthinking this and should simply indent the > > functions like I suggest. Diff tools can handle a bit of whitespace changing. > > I agree that putting the function header on a line with a different level of > indentation than the braces that encapsulate the function body looks quite > confusing. Neither do I recall having seen this in our code base before. > > Moreover, do I also find it inconsistent and difficult to parse if some > arguments are given on the line which initiated the function call while some are > wrapped in a way that it doesn't align with the initial arguments. > > As for consistency across our code base, here are two examples where we do it > like Dave and I are suggesting: > https://hg.adblockplus.org/adblockpluschrome/file/tip/chrome/devtools.js#l25 > https://hg.adblockplus.org/adblockpluschrome/file/tip/composer.js#l41 > > I cannot see any code in this file where the addional indentation would cause > exceeding the 80 characters limit, and if there is this would be an indication > that you probably shouldn't use an anonymous function there in the first place. Thanks Sebastian, done. (On a related note: I think it could be pretty helpful if we could work on some example code that demonstrates our code wrapping style. Figuring out how to wrap code can be a huge time sink, arguing about it even more. Ultimately it doesn't really matter, so we might as well standarise it more clearly. Is either one of you interested in setting that up? If not I might look into it one day.) https://codereview.adblockplus.org/29367181/diff/29370964/README.md File README.md (right): https://codereview.adblockplus.org/29367181/diff/29370964/README.md#newcode26 README.md:26: `npm test test/synchronizer.js`. On 2017/01/10 11:05:33, kzar wrote: > Mind also giving an example that runs both a unit test and a browser test file? > Just to make it extra clear for people new to the tests. Sure, I've just updated the one example to invoke multiple test files, should be obvious enough how it works I hope. This made me reconsider the approach to have a single runner (i.e. HTML file) for all browser tests. I have now changed that, so we can have one runner per suite. Otherwise it wouldn't be possible to specify individual browser tests to run from the command line. What do you think? https://codereview.adblockplus.org/29367181/diff/29370964/README.md#newcode32 README.md:32: opening `test/browser/index.html`. On 2017/01/10 11:05:33, kzar wrote: > Since the browser tests don't have a build step I guess we could make this a > link to the file. That way people could click on it to try the tests right away > in their browser from the README. (When it's rendered in GitHub at least.) Cool idea, done! https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js#newc... test_runner.js:55: else On 2017/01/10 11:05:33, kzar wrote: > Nit: Since the addTestPaths call goes over multiple lines we should add the > braces to the else clause. Since we're adding braces to the else clause we > should add it to the if clause too. Done. https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js#newc... test_runner.js:58: true); On 2017/01/10 11:05:34, kzar wrote: > Nit: We would normally put the closing parenthesis on the next line in line with > addTestsPaths here. We use the style above a lot, last time I checked at least, but changed it anyway. https://codereview.adblockplus.org/29367181/diff/29370964/test_runner.js#newc... test_runner.js:62: qunitFiles.forEach(file => qunit(path.resolve(file))); On 2017/01/10 11:05:34, kzar wrote: > Nit: We usually use "for of" loops instead of forEach where possible. I prefer forEach when it comes down to actually invoking a function for each element in an iterable. Here I had to put an anonymous function inbetween, so I've switched to for...of. (I realised the path.resolve is unnecessary, but since forEach passes on more than just the value, I couldn't go for `qunitFiles.forEach(qunit)`.)
https://codereview.adblockplus.org/29367181/diff/29370964/README.md File README.md (right): https://codereview.adblockplus.org/29367181/diff/29370964/README.md#newcode26 README.md:26: `npm test test/synchronizer.js`. On 2017/01/17 19:41:08, Felix Dahlke wrote: > On 2017/01/10 11:05:33, kzar wrote: > > Mind also giving an example that runs both a unit test and a browser test > file? > > Just to make it extra clear for people new to the tests. > > Sure, I've just updated the one example to invoke multiple test files, should be > obvious enough how it works I hope. > > This made me reconsider the approach to have a single runner (i.e. HTML file) > for all browser tests. I have now changed that, so we can have one runner per > suite. Otherwise it wouldn't be possible to specify individual browser tests to > run from the command line. What do you think? Oh, I had assumed that the node-qunit-phantomjs allowed you to run the browser tests by specifying a JavaScript file and that the runner was for when you wanted to run the tests from your browser. Yea, I agree it's better to have separate runners in that case. https://codereview.adblockplus.org/29367181/diff/29372245/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29372245/chrome/content/elem... chrome/content/elemHideEmulation.js:1: // We are currently limited to ECMAScript 5 in this file, because it is being Perhaps mentioning PhantomJS and the version in these comments would be useful? "We are currently limited to ECMAScript 5 in this file since the browser tests are run using PhantomJS 2.17. See https://issues.adblockplus.org/ticket/4796" https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... test/browser/elemHideEmulation.js:127: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], Mind also moving the first argument of the applyElemHideemulation calls down in line with the second? applyElemHideEmulation( ["[-abp-properties='background-color: rgb(0, 0, 0)']"], function() { ... } ); (That's generally how we do it, also it is nice to make the indentation consistent between the test cases.) https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... test/browser/elemHideEmulation.js:136: QUnit.test("Property selector with wildcard", function(assert) Mind bringing the function call and test description down onto the next line? That will make how the test case functions are indented consistent and as mentioned above that's generally how we do it. It's kind of unusual IMO to mix both styles of indentation. So like QUnit.test( "Property selector with wildcard", function(assert) { ... } );
Patch set 6 is up! https://codereview.adblockplus.org/29367181/diff/29372245/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29372245/chrome/content/elem... chrome/content/elemHideEmulation.js:1: // We are currently limited to ECMAScript 5 in this file, because it is being On 2017/01/18 04:17:15, kzar wrote: > Perhaps mentioning PhantomJS and the version in these comments would be useful? > > "We are currently limited to ECMAScript 5 in this file since the browser tests > are run using PhantomJS 2.17. See > https://issues.adblockplus.org/ticket/4796%22 The information is in the issue, I'd personally rather not duplicate (or rather "quadruplicate") it. https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... test/browser/elemHideEmulation.js:127: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/18 04:17:15, kzar wrote: > Mind also moving the first argument of the applyElemHideemulation calls down in > line with the second? > > applyElemHideEmulation( > ["[-abp-properties='background-color: rgb(0, 0, 0)']"], > function() > { > ... > } > ); > > (That's generally how we do it, also it is nice to make the indentation > consistent between the test cases.) I think there's a range of how we generally do this, but if you find this more readable (makes no relevant difference for me), I'll change it. Done. https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... test/browser/elemHideEmulation.js:136: QUnit.test("Property selector with wildcard", function(assert) On 2017/01/18 04:17:15, kzar wrote: > Mind bringing the function call and test description down onto the next line? > That will make how the test case functions are indented consistent and as > mentioned above that's generally how we do it. It's kind of unusual IMO to mix > both styles of indentation. > > So like > > QUnit.test( > "Property selector with wildcard", > function(assert) > { > ... > } > ); There's no need for the wrapping in these cases, and I don't think we generally wrap lines that don't have to be wrapped. IMO it hurts readability, but since I wrote it and you're reading it, you're the better judge of that. Done. (On the positive side, it does resolve the blame/diff issues I mentioned.)
LGTM
(Moving myself back to CC) https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... test/browser/elemHideEmulation.js:127: applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"], On 2017/01/19 09:56:51, Felix Dahlke wrote: > On 2017/01/18 04:17:15, kzar wrote: > > Mind also moving the first argument of the applyElemHideemulation calls down > in > > line with the second? > > > > applyElemHideEmulation( > > ["[-abp-properties='background-color: rgb(0, 0, 0)']"], > > function() > > { > > ... > > } > > ); > > > > (That's generally how we do it, also it is nice to make the indentation > > consistent between the test cases.) > > I think there's a range of how we generally do this, but if you find this more > readable (makes no relevant difference for me), I'll change it. Done. I agree with Dave. As I explained in my previous comment, it is harder to read if the arguments are not aligned. https://codereview.adblockplus.org/29367181/diff/29372245/test/browser/elemHi... test/browser/elemHideEmulation.js:136: QUnit.test("Property selector with wildcard", function(assert) On 2017/01/19 09:56:51, Felix Dahlke wrote: > On 2017/01/18 04:17:15, kzar wrote: > > Mind bringing the function call and test description down onto the next line? > > That will make how the test case functions are indented consistent and as > > mentioned above that's generally how we do it. It's kind of unusual IMO to mix > > both styles of indentation. > > > > So like > > > > QUnit.test( > > "Property selector with wildcard", > > function(assert) > > { > > ... > > } > > ); > > There's no need for the wrapping in these cases, and I don't think we generally > wrap lines that don't have to be wrapped. > > IMO it hurts readability, but since I wrote it and you're reading it, you're the > better judge of that. Done. (On the positive side, it does resolve the > blame/diff issues I mentioned.) FWIW, if all arguments fit on the same line as the function call, like here, I generally wouldn't wrap it. Here, one might argue though that it's more consistent to wrap all tests in the same way. I don't have a strong opinion. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
