1456972 - Add recording settings tests for new recording panel

Assignee

Description

•

6 years ago

There should be component tests and tests for the preferences.

Greg Tatum [:gregtatum]

Assignee

Updated

•

6 years ago

Priority: -- → P2

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/239812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239812/

Comment hidden (mozreview-request)

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239812/diff/1-2/

Julien Wajsberg [:julienw]

Comment 3

•

6 years ago

Should this review request go to Ola ?

Greg Tatum [:gregtatum]

Assignee

Updated

•

6 years ago

Attachment #8971060 - Flags: review?(felash) → review?(nchevobbe)

Nicolas Chevobbe [:nchevobbe]

Comment 4

•

6 years ago

mozreview-review

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

https://reviewboard.mozilla.org/r/239812/#review245772

Overall, this is looks good to me, thanks Greg :)
Could we go with more specific naming than test_perf-settings-0N.html ? It makes them hard to spot and maintain in the long run.

::: devtools/client/performance-new/test/chrome/head.js:140
(Diff revision 2)
> +function setReactFriendlyInputValue(element, value) {
> +  const valueSetter = Object.getOwnPropertyDescriptor(element, "value").set;
> +  const prototype = Object.getPrototypeOf(element);
> +  const prototypeValueSetter = Object.getOwnPropertyDescriptor(prototype, "value").set;
> +
> +  if (valueSetter && valueSetter !== prototypeValueSetter) {
> +    prototypeValueSetter.call(element, value);
> +  } else {
> +    valueSetter.call(element, value);
> +  }
> +  element.dispatchEvent(new Event("input", { bubbles: true }));
> +}

would focusing the input and simulating key strokes work ? I know the debugger uses this : https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/devtools/client/debugger/new/test/mochitest/head.js#970-972

it would better reflect "true" user usage

::: devtools/client/performance-new/test/chrome/head.js:172
(Diff revision 2)
>    const perfFront = new MockPerfFront();
>    const toolboxMock = {};
>    const store = createStore(reducers);
>    const container = document.querySelector("#container");
>    const receiveProfileCalls = [];
> +  const setRecordingPreferencesCalls = [];

could this be only `recordingPreferencesCalls` ?

::: devtools/client/performance-new/test/chrome/head.js:178
(Diff revision 2)
>  
>    function receiveProfileMock(profile) {
>      receiveProfileCalls.push(profile);
>    }
>  
> +  function setRecordingPreferences(settings) {

nit: instead of `set` say `push` or `add` which better reflet what's done here

::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:24
(Diff revision 2)
> +        const {
> +          perfFront,
> +          getRecordingState,
> +          mountComponent,
> +          selectors,
> +          getState,
> +          setRecordingPreferencesCalls
> +        } = createPerfComponent();
> +
> +        mountComponent();
> +        await perfFront.flushAsyncQueue();

this is repeated in the 4 tests. Would it make sense to put it behind a `bootstrapPerfComponent` (or something with a better name), that would return you the result of createPerfComponent ?

::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:34
(Diff revision 2)
> +          getState,
> +          setRecordingPreferencesCalls
> +        } = createPerfComponent();
> +
> +        mountComponent();
> +        await perfFront.flushAsyncQueue();

could we add a comment here explaining why we need to do this ?

::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:53
(Diff revision 2)
> +        is(setRecordingPreferencesCalls[0].interval, scaledValue,
> +          "The preference was recorded.");
> +
> +        // Now start the profiler.
> +        container.querySelector("button").click();
> +        await perfFront.flushAsyncQueue();

same here, it would be nice to explain why we need this

::: devtools/client/performance-new/test/chrome/test_perf-settings-02.html:53
(Diff revision 2)
> +        is(setRecordingPreferencesCalls[0].entries, scaledValue,
> +          "The preference was recorded.");
> +
> +        // Now start the profiler.
> +        container.querySelector("button").click();
> +        await perfFront.flushAsyncQueue();

a comment explaining why we need this would be nice.
Since it's also used in other tests, I guess this is needed each time you start the profiler, so maybe we could have a function in head.js that abstract this (`startProfiler(container)` ?)

::: devtools/client/performance-new/test/chrome/test_perf-settings-03.html:39
(Diff revision 2)
> +        await perfFront.flushAsyncQueue();
> +
> +        // Open up the features summary.
> +        document.querySelector("#perf-settings-features-summary").click();
> +
> +        is(selectors.getFeatures(getState()).join(","), "js,stackwalk",

does 

```js
is(selectors.getFeatures(getState()), ["js","stackwalk"], "The features...")
```

would work ? it might be a bit better when there are errors for the diffing (even if here we only have a couple of elements in the array)

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:40
(Diff revision 2)
> +
> +        // Open up the threads summary.
> +        document.querySelector("#perf-settings-threads-summary").click();
> +
> +        const threadTextEl = document.querySelector("#perf-settings-thread-text");
> +        is(selectors.getThreads(getState()).join(","), "GeckoMain,Compositor",

same question about the use of join here

Attachment #8971060 - Flags: review?(nchevobbe) → review+

Greg Tatum [:gregtatum]

Assignee

Comment 5

•

6 years ago

mozreview-review

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

https://reviewboard.mozilla.org/r/239812/#review245788

::: devtools/client/performance-new/test/chrome/head.js:140
(Diff revision 2)
> +function setReactFriendlyInputValue(element, value) {
> +  const valueSetter = Object.getOwnPropertyDescriptor(element, "value").set;
> +  const prototype = Object.getPrototypeOf(element);
> +  const prototypeValueSetter = Object.getOwnPropertyDescriptor(prototype, "value").set;
> +
> +  if (valueSetter && valueSetter !== prototypeValueSetter) {
> +    prototypeValueSetter.call(element, value);
> +  } else {
> +    valueSetter.call(element, value);
> +  }
> +  element.dispatchEvent(new Event("input", { bubbles: true }));
> +}

Hmm... I just checked it out, which looks like this would probably work for text inputs, but not for the range inputs, which require sliding the value left and right. I'm not completely happy with this function, but I think I'm going to keep it for now, (until we get enzyme, maybe!?)

I did do some work trying to send through events, and it wasn't working.

::: devtools/client/performance-new/test/chrome/head.js:178
(Diff revision 2)
>  
>    function receiveProfileMock(profile) {
>      receiveProfileCalls.push(profile);
>    }
>  
> +  function setRecordingPreferences(settings) {

Good point, this naming is confusing.

::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:34
(Diff revision 2)
> +          getState,
> +          setRecordingPreferencesCalls
> +        } = createPerfComponent();
> +
> +        mountComponent();
> +        await perfFront.flushAsyncQueue();

I added a new init function.

::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:53
(Diff revision 2)
> +        is(setRecordingPreferencesCalls[0].interval, scaledValue,
> +          "The preference was recorded.");
> +
> +        // Now start the profiler.
> +        container.querySelector("button").click();
> +        await perfFront.flushAsyncQueue();

I renamed things a bit and added more comments. Thanks for the feedback, this is not obvious what's going on in retrospect.

Comment hidden (mozreview-request)

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239812/diff/2-3/

Code Review Bot [:reviewbot]

Comment 7

•

6 years ago

mozreview-review

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

https://reviewboard.mozilla.org/r/239812/#review245842


Code analysis found 14 defects in this patch:
 - 14 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:26
(Diff revision 3)
> +       * Test that the profiler can set the interval settings.
> +       */
> +      addPerfTest(async () => {
> +        const {
> +          perfFrontMock,
> +          getRecordingState,

Error: 'getrecordingstate' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/performance-new/test/chrome/test_perf-settings-01.html:52
(Diff revision 3)
> +        is(recordingPreferencesCalls[0].interval, scaledValue,
> +          "The preference was recorded.");
> +
> +        // Start the profiler by clicking the start button, and flushing the async
> +        // calls out to the mock perf front.
> +        container.querySelector("button").click();

Error: 'container' is not defined. [eslint: no-undef]

::: devtools/client/performance-new/test/chrome/test_perf-settings-02.html:26
(Diff revision 3)
> +       * Test that the entries setting can be changed.
> +       */
> +      addPerfTest(async () => {
> +        const {
> +          perfFrontMock,
> +          getRecordingState,

Error: 'getrecordingstate' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/performance-new/test/chrome/test_perf-settings-02.html:52
(Diff revision 3)
> +        is(recordingPreferencesCalls[0].entries, scaledValue,
> +          "The preference was recorded.");
> +
> +        // Start the profiler by clicking the start button, and flushing the async
> +        // calls out to the mock perf front.
> +        container.querySelector("button").click();

Error: 'container' is not defined. [eslint: no-undef]

::: devtools/client/performance-new/test/chrome/test_perf-settings-03.html:26
(Diff revision 3)
> +       * Test that the features setting can be changed.
> +       */
> +      addPerfTest(async () => {
> +        const {
> +          perfFrontMock,
> +          getRecordingState,

Error: 'getrecordingstate' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/performance-new/test/chrome/test_perf-settings-03.html:52
(Diff revision 3)
> +          "The feature has been removed.");
> +        is(recordingPreferencesCalls.length, 1,
> +          "The preferences have been updated.");
> +        is(recordingPreferencesCalls[0].features.join(","), "stackwalk",
> +          "The preferences have been updated.");
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

::: devtools/client/performance-new/test/chrome/test_perf-settings-03.html:61
(Diff revision 3)
> +        is(selectors.getFeatures(getState()).join(","), "leaf,stackwalk",
> +          "Another feature was added");
> +
> +        // Start the profiler by clicking the start button, and flushing the async
> +        // calls out to the mock perf front.
> +        container.querySelector("button").click();

Error: 'container' is not defined. [eslint: no-undef]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:26
(Diff revision 3)
> +       * Test that the threads setting can be changed.
> +       */
> +      addPerfTest(async () => {
> +        const {
> +          perfFrontMock,
> +          getRecordingState,

Error: 'getrecordingstate' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:57
(Diff revision 3)
> +          "The threads have been updated in the thread text input");
> +        is(recordingPreferencesCalls.length, 1,
> +          "The preferences have been updated.");
> +        is(recordingPreferencesCalls[0].threads.join(","), "GeckoMain",
> +          "The preferences have been updated.");
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:67
(Diff revision 3)
> +          "Another thread was added");
> +        is(threadTextEl.value, "GeckoMain,DOM Worker",
> +          "Another thread was in the thread text input");
> +
> +        // See the initial state of the checkbox
> +        const styleThreadCheckbox = document.querySelector("#perf-settings-thread-checkbox-style-thread");

Error: Line 50 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:69
(Diff revision 3)
> +          "Another thread was in the thread text input");
> +
> +        // See the initial state of the checkbox
> +        const styleThreadCheckbox = document.querySelector("#perf-settings-thread-checkbox-style-thread");
> +        ok(!styleThreadCheckbox.checked,
> +          "The style thread is not checked.")

Error: Missing semicolon. [eslint: semi]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:76
(Diff revision 3)
> +        // Set the input box directly
> +        setReactFriendlyInputValue(threadTextEl, "GeckoMain,DOM Worker,StyleThread");
> +        threadTextEl.dispatchEvent(new Event("blur", { bubbles: true }));
> +
> +        ok(styleThreadCheckbox.checked,
> +          "The style thread is now checked.")

Error: Missing semicolon. [eslint: semi]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:84
(Diff revision 3)
> +        is(threadTextEl.value, "GeckoMain,DOM Worker,StyleThread",
> +          "Another thread was in the thread text input");
> +
> +        // Start the profiler by clicking the start button, and flushing the async
> +        // calls out to the mock perf front.
> +        container.querySelector("button").click();

Error: 'container' is not defined. [eslint: no-undef]

::: devtools/client/performance-new/test/chrome/test_perf-settings-04.html:89
(Diff revision 3)
> +        container.querySelector("button").click();
> +        await perfFrontMock._flushAsyncQueue();
> +
> +        is(perfFrontMock._startProfilerCalls.length, 1,
> +          "Start profiler was called once");
> +        is(perfFrontMock._startProfilerCalls[0].threads.join(","), "GeckoMain,DOM Worker,StyleThread",

Error: Line 72 exceeds the maximum line length of 90. [eslint: max-len]

Comment hidden (mozreview-request)

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239812/diff/3-4/

Greg Tatum [:gregtatum]

Assignee

Comment 9

•

6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=361bc4a5bff36e32b90324874b8b657a91790c3b

Greg Tatum [:gregtatum]

Assignee

Comment 10

•

6 years ago

The above job is testing out the L10N failure, to see if it reproduces on the latest tip. Seems pretty mysterious to me.

Comment hidden (mozreview-request)

Comment on attachment 8971060 [details]
Bug 1456972 - Add recording settings tests for perf recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239812/diff/4-5/

Pulsebot

Comment 12

•

6 years ago

Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07e342719f5e
Add recording settings tests for perf recording panel; r=nchevobbe

Natalia Csoregi [:nataliaCs]

Comment 13

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/07e342719f5e

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox62: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 62

Sylvestre Ledru [:Sylvestre]

Updated

•

6 years ago

Assignee: nobody → gtatum

BMO Automation

Updated

•

6 years ago

Product: Firefox → DevTools

Bugzilla

Quick Search

Add recording settings tests for new recording panel

Categories

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P2)

Tracking

(firefox62 fixed)

People

(Reporter: gregtatum, Assigned: gregtatum)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Updated

Comment 1

Comment 2

Comment 3

Updated

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Updated

Updated

Attachment

General

Description

File Name

Content Type