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)

Product:
Core
Component:
Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: cfu, Assigned: cfu)

References

Details

(Whiteboard: [tor][fingerprinting])

Attachments

(1 file)

Bug 1406294 - Refactor browser/components/resistfingerprinting/test/browser rounded window tests.
59 bytes, text/x-review-board-request
timhuang
: review+
smaug
: review+
Details 
Preparation for these intermittent bugs:
Bug 1355717
Bug 1356257
Bug 1362249
Bug 1352661
Bug 1373139
Bug 1382630
Assignee: nobody → cfu
Blocks: 1355717, 1356257, 1362249, 1352661, 1373139, 1382630
Attachment #8915921 - Flags: review?(tihuang)
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.
Priority: -- → P2
Whiteboard: [tor][fingerprinting]
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+
Attachment #8915921 - Flags: review?(bugs)
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!
(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 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-
(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 :)
(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?
(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);
ok, fine. Then document that the param is optional for run() and compare to undefined and not to null.
Thank you very much :)  Please take a look again.
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+
Keywords: checkin-needed
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: