Issue 29823569: fixed ioIndexedDB tests (Closed)
|
Created:
July 5, 2018, 7:27 a.m. by piscoi.georgiana Modified:
July 9, 2018, 6:40 p.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionFixed ioIndexedDB tests that were introduced with issue 6621
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : reverted unrelated code changes #
Total comments: 2
Patch Set 4 : #MessagesTotal messages: 10
I've updated the tests, so that they will run only on Edge, and skip them for other browsers. Also addressed https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex...
https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (left): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:53: test("writeToFile", assert => Seems like a bunch of unrelated changes here? I'd prefer it if you just changed (more or less) what was necessary for the task at hand. That would make it easier to review. https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (right): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:27: if (isEdge) I don't think this check (and the one in afterEach) is necessary since the tests are skipped anyway.
https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (left): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:53: test("writeToFile", assert => On 2018/07/05 09:54:15, kzar wrote: > Seems like a bunch of unrelated changes here? I'd prefer it if you just changed > (more or less) what was necessary for the task at hand. That would make it > easier to review. Sorry about that. In the future I'll leave this sort of changes for last, but I still want them in, as the intention was to improve code locality. https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (right): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:27: if (isEdge) On 2018/07/05 09:54:15, kzar wrote: > I don't think this check (and the one in afterEach) is necessary since the tests > are skipped anyway. You are right, I don't know why I was under the impression that those 2 functions are called. Either way, I've removed the checks.
https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (left): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:53: test("writeToFile", assert => On 2018/07/09 14:04:31, geo wrote: > On 2018/07/05 09:54:15, kzar wrote: > > Seems like a bunch of unrelated changes here? I'd prefer it if you just > changed > > (more or less) what was necessary for the task at hand. That would make it > > easier to review. > > Sorry about that. In the future I'll leave this sort of changes for last, but I > still want them in, as the intention was to improve code locality. Sure, no problem. FWIW I agree that it's good to improve code locality where possible. Would you mind removing those unrelated changes from this code review though? I think they belong in a separate review (and commit). Don't worry it doesn't require filing another issue, for more trivial changes like those we label the commit "Noissue - ...". https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (right): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:27: if (isEdge) On 2018/07/09 14:04:31, geo wrote: > On 2018/07/05 09:54:15, kzar wrote: > > I don't think this check (and the one in afterEach) is necessary since the > tests > > are skipped anyway. > > You are right, I don't know why I was under the impression that those 2 > functions are called. Either way, I've removed the checks. Acknowledged.
I've reverted some of the code changes introduced in p1, while keeping changes from p2. https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (left): https://codereview.adblockplus.org/29823569/diff/29823570/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:53: test("writeToFile", assert => On 2018/07/09 14:17:28, kzar wrote: > On 2018/07/09 14:04:31, geo wrote: > > On 2018/07/05 09:54:15, kzar wrote: > > > Seems like a bunch of unrelated changes here? I'd prefer it if you just > > changed > > > (more or less) what was necessary for the task at hand. That would make it > > > easier to review. > > > > Sorry about that. In the future I'll leave this sort of changes for last, but > I > > still want them in, as the intention was to improve code locality. > > Sure, no problem. FWIW I agree that it's good to improve code locality where > possible. > > Would you mind removing those unrelated changes from this code review though? I > think they belong in a separate review (and commit). Don't worry it doesn't > require filing another issue, for more trivial changes like those we label the > commit "Noissue - ...". Done.
https://codereview.adblockplus.org/29823569/diff/29825672/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (right): https://codereview.adblockplus.org/29823569/diff/29825672/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:22: QUnit.module("Edge filter storage", { Nit: We always spell out "Microsoft Edge" to avoid any confusion. Otherwise, the changes look pretty good. Thanks!
https://codereview.adblockplus.org/29823569/diff/29825672/qunit/tests/ioIndex... File qunit/tests/ioIndexedDB.js (right): https://codereview.adblockplus.org/29823569/diff/29825672/qunit/tests/ioIndex... qunit/tests/ioIndexedDB.js:22: QUnit.module("Edge filter storage", { On 2018/07/09 16:43:39, Sebastian Noack wrote: > Nit: We always spell out "Microsoft Edge" to avoid any confusion. > > Otherwise, the changes look pretty good. Thanks! Done. Thank you!
LGTM
LGTM
Pushed: https://hg.adblockplus.org/adblockpluschrome/rev/6609decc0dc4 Feel free to close this review. Thanks! 
|
