Closed
Bug 1406294
Opened 7 years ago
Closed 7 years ago
Refactor browser/components/resistfingerprinting/test/browse rounded window tests
Categories
(Core :: Security, enhancement, P2)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: cfu, Assigned: cfu)
References
Details
(Whiteboard: [tor][fingerprinting])
Attachments
(1 file)
Preparation for these intermittent bugs: Bug 1355717 Bug 1356257 Bug 1362249 Bug 1352661 Bug 1373139 Bug 1382630
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8915921 -
Flags: review?(tihuang)
Assignee | ||
Comment 2•7 years ago
|
||
I'm going to split these tests into small parts. In order not to generate more redundant code, I first make the tests clean, moving same code together.
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [tor][fingerprinting]
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8915921 [details] Bug 1406294 - Refactor browser/components/resistfingerprinting/test/browser rounded window tests. https://reviewboard.mozilla.org/r/187182/#review192596 LGTM. Good job, thanks for your help on this. But there are some minor issues here. ::: browser/components/resistfingerprinting/test/browser/head.js:234 (Diff revision 1) > ); > } > + > +class RoundedWindowTest { > + static run(testCases, testOuter) { > + // When invoked by ConcreteTest.run(), this is class ConcreteTest itself. I don't really get what you want to descirbe by 'ConcreteTest' here. Does the 'ConcreteTest' refer to 'WindowSettingTest' or 'OpenTest' here? If it is, why don't you use 'Subclasses'? ::: browser/components/resistfingerprinting/test/browser/head.js:285 (Diff revision 1) > + for (let test of this.testCases) { > + await this.doTest(test, testOuter); > + } > + > + await BrowserTestUtils.removeTab(this.tab); > + } Could you add something like ... ``` doTest() { throw new Error('Subclasses of RoundedWindowTest must implement this to do the test.'); } ``` For better understanding of how subclasses work with RoundedWindowTest. And this also prevents the potential problem that calling an undefined as a function when RoundedWindowTest.run() be called directly, although I don't think this is the case. But, it would be good if you add this. :)
Attachment #8915921 -
Flags: review?(tihuang) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8915921 -
Flags: review?(bugs)
Assignee | ||
Comment 5•7 years ago
|
||
Hi smaug, Could I also ask for your help with the intermittent bugs? The changes will be trivial that split a single test Test.run(testCases); into 2 tests, calling Test.run(testCases, false); and Test.run(testCases, true); respectively. Although they will be small patches, I'm afraid it annoys you too much (there are 6 bugs). If you think it will take too much of your time, could you please recommend someone else whom we can ask for help? Thank you very much!
Comment 6•7 years ago
|
||
(In reply to Chung-Sheng Fu [:cfu] from comment #5) > Hi smaug, > > Could I also ask for your help with the intermittent bugs? > > The changes will be trivial that split a single test > > Test.run(testCases); > > into 2 tests, calling > > Test.run(testCases, false); > > and > > Test.run(testCases, true); > > respectively. > > Although they will be small patches, I'm afraid it annoys you too much Heh, I'm sure it doesn't. But what does testOuter mean on run() method? Apparently it can be null or true or false. Could you add some comment there. Does anyone ever pass true or false, or even null to run(). Don't all the caller just not pass anything, which mean the value is undefined?
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8915921 [details] Bug 1406294 - Refactor browser/components/resistfingerprinting/test/browser rounded window tests. https://reviewboard.mozilla.org/r/187182/#review192632 I think I'd like to see run() method some cleaning, since I see it being called only with one param, yet the code can deal with two.
Attachment #8915921 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6) > (In reply to Chung-Sheng Fu [:cfu] from comment #5) > Heh, I'm sure it doesn't. > > > But what does testOuter mean on run() method? > Apparently it can be null or true or false. > Could you add some comment there. > Does anyone ever pass true or false, or even null to run(). Don't all the > caller just not pass anything, which mean the value is undefined? It will be passed to the test bodies: testWindowOpen http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/browser/components/resistfingerprinting/test/browser/head.js#100 testWindowSizeSetting http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/browser/components/resistfingerprinting/test/browser/head.js#155 I'm not sure how to name it better, so I kept its name "aTestOuter" from testWindowOpen and testWindowSizeSetting. It determines whether the test is going to check window.innerWidth/innerHeight or window.outerWidth/outerHeight. Basically it should be either true or false, but I would like to keep the tests doing both inner and outer in one test before I split them, so I tried to use "== null" to present the third state. Not passing any argument will lead to undefined, which satisfies the "== null" condition and does tests for both inner and outer. To make it clearer, I may explicitly pass null to the run() method like Test.run(testCases, null); Another approach may be using an int value, for example: 0 - inner, 1 - outer, 2 - both. Which looks better to you? I sincerely appreciate your help, you are really so nice :)
Comment 9•7 years ago
|
||
(In reply to Chung-Sheng Fu [:cfu] from comment #8) > To make it clearer, I may explicitly pass null to the run() method like > > Test.run(testCases, null); No. Just compare to undefined and not null. No one is passing null, so why to compare to it. But I still don't see who is passing second param to run() method. The links you gave use that value which run() method itself passes to those methods, but who calls run() with more than one param?
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9) > (In reply to Chung-Sheng Fu [:cfu] from comment #8) > > > To make it clearer, I may explicitly pass null to the run() method like > > > > Test.run(testCases, null); > > No. Just compare to undefined and not null. No one is passing null, so why > to compare to it. > > > But I still don't see who is passing second param to run() method. > The links you gave use that value which run() method itself passes to those > methods, but who calls run() with more than one param? It it not used in this bug but reserved for those intermittent bugs. In order to reduce the execution time, I'm going to split the tests. So I will need this param to do a smaller test set. Take browser_roundedWindow_open_max.js for example, it now looks like OpenTest.run([ {settingWidth: 1025, settingHeight: 1050, targetWidth: 1000, targetHeight: 1000}, {settingWidth: 9999, settingHeight: 9999, targetWidth: 1000, targetHeight: 1000}, {settingWidth: 999, settingHeight: 999, targetWidth: 1000, targetHeight: 1000} ]); I will split it to 2 files, maybe browser_roundedWindow_open_max_inner.js and browser_roundedWindow_open_max_outer.js, and they will pass the testOuter param like browser_roundedWindow_open_max_inner.js: OpenTest.run([ {settingWidth: 1025, settingHeight: 1050, targetWidth: 1000, targetHeight: 1000}, {settingWidth: 9999, settingHeight: 9999, targetWidth: 1000, targetHeight: 1000}, {settingWidth: 999, settingHeight: 999, targetWidth: 1000, targetHeight: 1000} ], false); browser_roundedWindow_open_max_outer.js: OpenTest.run([ {settingWidth: 1025, settingHeight: 1050, targetWidth: 1000, targetHeight: 1000}, {settingWidth: 9999, settingHeight: 9999, targetWidth: 1000, targetHeight: 1000}, {settingWidth: 999, settingHeight: 999, targetWidth: 1000, targetHeight: 1000} ], true);
Comment 11•7 years ago
|
||
ok, fine. Then document that the param is optional for run() and compare to undefined and not to null.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Thank you very much :) Please take a look again.
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8915921 [details] Bug 1406294 - Refactor browser/components/resistfingerprinting/test/browser rounded window tests. https://reviewboard.mozilla.org/r/187182/#review193542
Attachment #8915921 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0802c1c711ef Refactor browser/components/resistfingerprinting/test/browser rounded window tests. r=smaug,timhuang
Keywords: checkin-needed
![]() |
||
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0802c1c711ef
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•