Fix ESLint issues in devtools/server/tests/browser/

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ntim, Assigned: Ruturaj Vartak)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
Firefox 54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
To view ESLint issues:

./mach eslint devtools/server/tests/browser/ --no-ignore --fix


You'll need to fix them manually afterwards.

Recommendation: Adding a .eslintrc.js file in the directory will reduce the amount of errors.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/test/.eslintrc.js
(Reporter)

Updated

a year ago
Summary: Fix ESLint isues in devtools/server/tests/browser/ → Fix ESLint issues in devtools/server/tests/browser/
Priority: -- → P3
(Assignee)

Updated

a year ago
Assignee: nobody → ruturaj
(Assignee)

Comment 1

a year ago
While I'm at it, How am I supposed to handle following issue
--------------------------------------------------------
$ ./mach eslint devtools/server/tests/browser/browser_a* --no-ignore
/home/rutu/code/gecko-dev/devtools/server/tests/browser/browser_animation_refreshTransitions.js
  24:14  error  content is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

✖ 1 problem (1 error, 0 warnings)
--------------------------------------------------------

I read [1] , couldn't make out how I could work around that.

[1] http://gfritzsche-demo.readthedocs.io/en/latest/tools/lint/eslint/eslint-plugin-mozilla/eslint-plugin-mozilla/no-cpows-in-tests.html
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 2

a year ago
(In reply to Ruturaj Vartak from comment #1)
> While I'm at it, How am I supposed to handle following issue
> --------------------------------------------------------
> $ ./mach eslint devtools/server/tests/browser/browser_a* --no-ignore
> /home/rutu/code/gecko-dev/devtools/server/tests/browser/
> browser_animation_refreshTransitions.js
>   24:14  error  content is a possible Cross Process Object Wrapper (CPOW). 
> mozilla/no-cpows-in-tests (eslint)
> 
> ✖ 1 problem (1 error, 0 warnings)
> --------------------------------------------------------
> 
> I read [1] , couldn't make out how I could work around that.
> 
> [1]
> http://gfritzsche-demo.readthedocs.io/en/latest/tools/lint/eslint/eslint-
> plugin-mozilla/eslint-plugin-mozilla/no-cpows-in-tests.html
Thanks for your interest in this bug.

To fix the CPOW error, you should:
Replace:
  let cpow = content.document.querySelector(".all-transitions");
  cpow.classList.add("expand");

with:
  yield ContentTask.spawn(gBrowser.selectedBrowser, {}, () => {
    let el = content.document.querySelector(".all-transitions");
    el.classList.add("expand");
  });


and replace:
  cpow.classList.remove("expand");

with:
  yield ContentTask.spawn(gBrowser.selectedBrowser, {}, () => {
    let el = content.document.querySelector(".all-transitions");
    el.classList.remove("expand");
  });
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 3

a year ago
Thanks Tim,

Could you also explain or point to any documentation related to it.

ie. why CPOW is looked down, and how the other work around is better?
It'll help me to fix the issues better rather than just asking you guys.

Thanks
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 4

a year ago
(In reply to Ruturaj Vartak from comment #3)
> Thanks Tim,
> 
> Could you also explain or point to any documentation related to it.
> 
> ie. why CPOW is looked down, and how the other work around is better?
> It'll help me to fix the issues better rather than just asking you guys.
Firefox now has a multi-process architecture, so parent and child processes are separated, which means direct synchronous access to `content` (child process) from the parent process (CPOW) is deprecated. ContentTask is a test helper that allows accessing the child process in a more safe asynchronous way.

More info about cpows: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers

ESLint documentation: http://eslint.org/
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 5

a year ago
Hey Tim,

Do you propose any specific way to handle duplicate case statements in here https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/browser_navigateEvents.js#20 ?

Wondering if we can use variable "event" and setup cases like
````
case "request": ...
case "load-new-document": ...
`````
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 6

a year ago
(In reply to Ruturaj Vartak from comment #5)
> Hey Tim,
> 
> Do you propose any specific way to handle duplicate case statements in here
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/
> browser_navigateEvents.js#20 ?
> 
> Wondering if we can use variable "event" and setup cases like
> ````
> case "request": ...
> case "load-new-document": ...
> `````

That wouldn't work for 2 reasons:
- It'd no longer check for the event order
- There are some events occurring twice with different data


I see 2 potential ways to fix those statements:
- Using generator functions - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*
Although it would require rewriting a lot of code.
 
- Removing x, and replacing case x++: with case 0/1/2/3/4/...
Seems like the simplest solution here.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 7

a year ago
For the file, https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/browser/browser_canvasframe_helper_01.js#24

I was trying to make NodeBuilder sync by doing something like this 

==============
  let nodeBuilder = () => {
    return (function* () {
      yield ContentTask.spawn(gBrowser.selectedBrowser, {}, () => {
        let doc1 = content.document;
        el.classList.add("expand");

        let root = doc1.createElement("div");
        let child = doc1.createElement("div");
        child.style = "width:200px;height:200px;background:red;";
        child.id = "child-element";
        child.className = "child-element";
        child.textContent = "test element";
        root.appendChild(child);
      });
      yield root;
    })().next().next().value;

    // return root;
  };
==============

Obviously it hasn't worked, gives the following error
=====================
7 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_01.js | Uncaught exception - [object CPOW [Exception... "It's illegal to pass a CPOW to native code arg 0 [nsIWebProgress.addProgressListener]"  nsresult: "0x80570036 (NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE)"  location: "<unknown>"  data: no]]
Stack trace:
    Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59

=============================
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 8

a year ago
Unfortunately, you can't wrap this with ContentTask.spawn, because the child process needs access to some imports in the parent process. This case would require some significant changes to the existing code (which are not worth the effort for this bug), so feel free to ignore it. I would just add `// eslint-disable-next-line mozilla/no-cpows-in-tests` before the relevant CPOW.

CPOWs other than browser_canvasframe_helper_*.js should be trivial to remove though, but feel free to post a WIP patch if you need help removing them.

Also, just an aside if you're curious, to make some async code look synchronous, there are two ways to do so:

- Async functions

async function foo() {
  let value = await ContentTask.spawn(...);
  return value;
}


- Task.js (currently preferred way, because async functions need more testing in gecko):

let foo = Task.async(function* () {
  let value = yield ContentTask.spawn(...);
  return value;
});
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 9

a year ago
Created attachment 8839331 [details] [diff] [review]
WIP-fix-1325987.patch

Current state of code / patch passes all tests

- ESLint fixes

Unsure how to fix following
- CPOW in browser_storage_updates.js
- CPOW in browser_canvasframe_helper_04.js

- I tried solving `browser_timeline.js` in the way you suggested , like..
yield ContentTask.spawn(gBrowser.selectedBrowser, {}, () => {
    ...
});
The individual browser_timeline.js test passed, but overall testfailed
`./mach test devtools/server/tests/browser/`

- I've got to yet fix duplicate label (browser_navigateEvents.js)
Attachment #8839331 - Flags: review?(ntim.bugs)
(Reporter)

Comment 10

a year ago
Comment on attachment 8839331 [details] [diff] [review]
WIP-fix-1325987.patch

Review of attachment 8839331 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/tests/browser/animation.html
@@ +165,2 @@
>    // Get the transitions started when the page loads
> +  // var players;

You simply remove it if it's unused.

::: devtools/server/tests/browser/browser_animation_getFrames.js
@@ +6,5 @@
> +
> +// Check that the AnimationPlayerActor exposes a getFrames method that returns
> +// the list of keyframes in the animation.
> +
> +// const URL = MAIN_DOMAIN + "animation.html";

Same here.

::: devtools/server/tests/browser/browser_canvasframe_helper_01.js
@@ +19,4 @@
>  
>  add_task(function* () {
>    let browser = yield addTab(TEST_URL);
> +  // `doc` is used in a callback, so...

You don't need to include this comment.

::: devtools/server/tests/browser/browser_canvasframe_helper_04.js
@@ +10,4 @@
>  // This makes sure the 'domnode' protocol actor type is known when importing
>  // highlighter.
>  require("devtools/server/actors/inspector");
> +// const events = require("sdk/event/core");

Please remove it if it's unused

@@ +18,5 @@
>    CanvasFrameAnonymousContentHelper
>  } = require("devtools/server/actors/highlighters/utils/markup");
>  
> +// const TEST_URL_1
> +//   = "data:text/html;charset=utf-8,CanvasFrameAnonymousContentHelper test 1";

Same here.

@@ +20,5 @@
>  
> +// const TEST_URL_1
> +//   = "data:text/html;charset=utf-8,CanvasFrameAnonymousContentHelper test 1";
> +const TEST_URL_2
> +  = "data:text/html;charset=utf-8,CanvasFrameAnonymousContentHelper test 2";

Can you rename TEST_URL_2 to simply be TEST_URL ?

::: devtools/server/tests/browser/browser_directorscript_actors.js
@@ +8,4 @@
>  const {DirectorRegistry} = require("devtools/server/actors/director-registry");
>  
>  add_task(function* () {
> +  // let browser = yield addTab(MAIN_DOMAIN + "director-script-target.html");

This is what seems to be causing the tests to fail.

You're not adding a tab anymore, so the test is timing out.

You might want to replace this line by:
  yield addTab(MAIN_DOMAIN + "director-script-target.html");

@@ +8,5 @@
>  const {DirectorRegistry} = require("devtools/server/actors/director-registry");
>  
>  add_task(function* () {
> +  // let browser = yield addTab(MAIN_DOMAIN + "director-script-target.html");
> +  // let doc = browser.contentDocument;

You can remove let doc = ...; since it's unused.

@@ +54,2 @@
>        exports.attach = function ({port}) {
>          port.onmessage = function (evt) {

You can fix the no-shadow error by doing:

 exports.attach = function ({port: messagePort}) {
   messagePort.onmessage = function (evt) {

If you're interested about how the syntax works:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

@@ +87,2 @@
>        exports.attach = function ({window, port}) {
> +        let onpageloaded = function () {

Same here. You can use:

function ({window, port: evalPort}) {

@@ +124,1 @@
>        exports.attach = function ({port, onUnload}) {

Same here:

      exports.attach = function ({port: unloadPort, onUnload}) {

::: devtools/server/tests/browser/browser_directorscript_actors_error_events.js
@@ +8,4 @@
>  const {DirectorRegistry} = require("devtools/server/actors/director-registry");
>  
>  add_task(function* () {
> +  // let browser = yield addTab(MAIN_DOMAIN + "director-script-target.html");

We still want to call addTab, so the right fix is replacing the line with:

  yield addTab(MAIN_DOMAIN + "director-script-target.html");

@@ +8,5 @@
>  const {DirectorRegistry} = require("devtools/server/actors/director-registry");
>  
>  add_task(function* () {
> +  // let browser = yield addTab(MAIN_DOMAIN + "director-script-target.html");
> +  // let doc = browser.contentDocument;

doc can be removed.

::: devtools/server/tests/browser/browser_directorscript_actors_exports.js
@@ +10,5 @@
>  DirectorRegistry.clear();
>  
>  add_task(function* () {
> +  // let browser = yield addTab(MAIN_DOMAIN + "director-script-target.html");
> +  // let doc = browser.contentDocument;

Same here.

::: devtools/server/tests/browser/browser_markers-cycle-collection.js
@@ +13,5 @@
>    // This test runs very slowly on linux32 debug EC2 instances.
>    requestLongerTimeout(2);
>  
> +  yield addTab(MAIN_DOMAIN + "doc_force_cc.html");
> +  // let doc = browser.contentDocument;

Please remove // let doc = ...;

::: devtools/server/tests/browser/browser_markers-docloading-01.js
@@ +10,5 @@
>  const MARKER_NAMES = ["document::DOMContentLoaded", "document::Load"];
>  
>  add_task(function* () {
>    let browser = yield addTab(MAIN_DOMAIN + "doc_innerHTML.html");
> +  // `doc` is used in a callback, so...

You don't need to include this comment.

::: devtools/server/tests/browser/browser_markers-docloading-02.js
@@ +10,5 @@
>  const MARKER_NAMES = ["document::DOMContentLoaded", "document::Load"];
>  
>  add_task(function* () {
>    let browser = yield addTab(MAIN_DOMAIN + "doc_innerHTML.html");
> +  // `doc` is used in a callback, so...

Same here.

::: devtools/server/tests/browser/browser_markers-docloading-03.js
@@ +10,5 @@
>  const MARKER_NAMES = ["document::DOMContentLoaded", "document::Load"];
>  
>  add_task(function* () {
>    let browser = yield addTab(MAIN_DOMAIN + "doc_innerHTML.html");
> +  // `doc` is used in a callback, so...

and here.

::: devtools/server/tests/browser/browser_markers-gc.js
@@ +10,5 @@
>  const MARKER_NAME = "GarbageCollection";
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_force_gc.html");
> +  // let doc = browser.contentDocument;

You can remove this comment.

::: devtools/server/tests/browser/browser_markers-parse-html.js
@@ +10,5 @@
>  const MARKER_NAME = "Parse HTML";
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_innerHTML.html");
> +  // let doc = browser.contentDocument;

Same here.

::: devtools/server/tests/browser/browser_markers-styles.js
@@ +10,5 @@
>  const MARKER_NAME = "Styles";
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

and here.

::: devtools/server/tests/browser/browser_markers-timestamp.js
@@ +12,5 @@
>  const MARKER_NAME = "TimeStamp";
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

and here as well.

@@ +26,5 @@
>    pmmConsoleMethod("timeStamp");
>    pmmConsoleMethod("timeStamp", "myLabel");
>  
> +  let markers = yield waitForMarkerType(front, MARKER_NAME,
> +    markers2 => markers2.length >= 2);

I'd prefer naming it `m` since it fits nicely into one line:

let markers = yield waitForMarkerType(front, MARKER_NAME, m => m.length >= 2);

::: devtools/server/tests/browser/browser_navigateEvents.js
@@ +119,5 @@
>  
>  function test() {
>    // Open a test tab
>    addTab(URL1).then(function (browser) {
> +    // let doc = browser.contentDocument;

You can remove this comment.

::: devtools/server/tests/browser/browser_perf-allocation-data.js
@@ +10,5 @@
>  const { PerformanceFront } = require("devtools/shared/fronts/performance");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_allocations.html");
> +  // let doc = browser.contentDocument;

Same here.

::: devtools/server/tests/browser/browser_perf-profiler-01.js
@@ +14,4 @@
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

and here.

::: devtools/server/tests/browser/browser_perf-realtime-markers.js
@@ +10,5 @@
>  const { PerformanceFront } = require("devtools/shared/fronts/performance");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

here as well

::: devtools/server/tests/browser/browser_perf-recording-actor-01.js
@@ +11,5 @@
>  const { PerformanceFront } = require("devtools/shared/fronts/performance");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

here too.

::: devtools/server/tests/browser/browser_perf-recording-actor-02.js
@@ +13,5 @@
>  const { PerformanceFront } = require("devtools/shared/fronts/performance");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

You can also remove the comment here.

::: devtools/server/tests/browser/browser_perf-samples-01.js
@@ +14,5 @@
>  const { PerformanceFront } = require("devtools/shared/fronts/performance");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

Same here.

::: devtools/server/tests/browser/browser_perf-samples-02.js
@@ +15,5 @@
>  const { PerformanceFront } = require("devtools/shared/fronts/performance");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "doc_perf.html");
> +  // let doc = browser.contentDocument;

Same here.

::: devtools/server/tests/browser/browser_register_actor.js
@@ +26,2 @@
>        registry.registerActor(actorURL, options).then(actorFront => {
> +        gClient.listTabs(response2 => {

Can you name this variable `res` or `resp` ?

@@ +29,4 @@
>            ok(!!tab.helloActor, "Hello actor must exist");
>  
>            // Make sure actor's state is maintained across listTabs requests.
> +          checkActorState(tab.helloActor, checkActorCleanup.bind(this, actorFront));

a more elegant way to do this is:

checkActorState(tab.helloActor, () => cleanupActor(actorFront));

@@ +34,5 @@
>        });
>      });
>  }
>  
> +function checkActorCleanup(actorFront) {

This function is not actually checking for cleanup, it's just cleaning up, so I would call it cleanup or cleanupActor

@@ +58,4 @@
>          is(tab.helloActor, helloActor, "Hello actor must be valid");
>  
> +        // The countCheck will get called with 3rd argument as the response
> +        getCount(helloActor, countCheck.bind(this, 3, callback));

A better way to do this would be making getCount() return a Promise:

function getCount(actor, callback) {
  return gClient.request({
    to: actor,
    type: "count"
  }, callback);
}

and then:

var checkActorState = Task.async(function* (helloActor, callback) {
  let response = yield getCount(helloActor);
  ok(!response.error, "No error");	
  is(response.count, 1, "The counter must be valid");

  response = yield getCount(helloActor);
  ok(!response.error, "No error");	
  is(response.count, 2, "The counter must be valid");

  let {tabs, selected} = yield gClient.listTabs();
  let tab = tabs[selected];
  is(tab.helloActor, helloActor, "Hello actor must be valid");

  response = yield getCount(helloActor);
  ok(!response.error, "No error");
  is(response.count, 3 "The counter must be valid");

  callback();
});

::: devtools/server/tests/browser/browser_storage_listings.js
@@ +337,3 @@
>  
> +//   };
> +// }

You can get rid of this.

::: devtools/server/tests/browser/browser_storage_updates.js
@@ +8,5 @@
> +// const beforeReload = {
> +//   cookies: ["http://test1.example.org", "https://sectest1.example.org"],
> +//   localStorage: ["http://test1.example.org", "http://sectest1.example.org"],
> +//   sessionStorage: ["http://test1.example.org", "http://sectest1.example.org"],
> +// };

You can remove this comment too.

::: devtools/server/tests/browser/browser_stylesheets_nested-iframes.js
@@ +10,5 @@
>  const {StyleSheetsFront} = require("devtools/shared/fronts/stylesheets");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "stylesheets-nested-iframes.html");
> +  // let doc = browser.contentDocument;

Same here.

::: devtools/server/tests/browser/browser_timeline_actors.js
@@ +10,5 @@
>  const {TimelineFront} = require("devtools/shared/fronts/timeline");
>  
>  add_task(function* () {
> +  yield addTab("data:text/html;charset=utf-8,mop");
> +  // let doc = browser.contentDocument;

and here.

::: devtools/server/tests/browser/browser_timeline_iframes.js
@@ +10,5 @@
>  const {TimelineFront} = require("devtools/shared/fronts/timeline");
>  
>  add_task(function* () {
> +  yield addTab(MAIN_DOMAIN + "timeline-iframe-parent.html");
> +  // let doc = browser.contentDocument;

and here too.

::: devtools/server/tests/browser/director-script-target.html
@@ +7,4 @@
>        window.eval = function () {
>          return "unsecure-eval-called";
>        };
> +      // var globalAccessibleVar = "global-value";

You probably want to keep this variable since one test relies on it.

Just use /* exported globalAccessibleVar */

::: devtools/server/tests/browser/doc_force_cc.html
@@ +10,5 @@
>  
>    <body>
>      <script type="text/javascript">
> +    "use strict";
> +    

nit: leading whitespace
Attachment #8839331 - Flags: review?(ntim.bugs)
(Reporter)

Comment 11

a year ago
(In reply to Ruturaj Vartak from comment #9)
> Created attachment 8839331 [details] [diff] [review]
> WIP-fix-1325987.patch
> 
> Current state of code / patch passes all tests
> 
> - ESLint fixes
> 
> Unsure how to fix following
> - CPOW in browser_storage_updates.js
> - CPOW in browser_canvasframe_helper_04.js
You can ignore these two using eslint-disable-line. They require a lot of rewriting.

> - I tried solving `browser_timeline.js` in the way you suggested , like..
> yield ContentTask.spawn(gBrowser.selectedBrowser, {}, () => {
>     ...
> });
> The individual browser_timeline.js test passed, but overall testfailed
> `./mach test devtools/server/tests/browser/`
That might be because of the issues in other tests I pointed out in the review.

> - I've got to yet fix duplicate label (browser_navigateEvents.js)

I wanted to test my suggestions from comment 6, but the test itself without modifications times out on my machine unfortunately. Feel free to use (I might have gotten the rule name wrong):

/* eslint-disable no-duplicate-label */
switch (...) {
 ...

}
/* eslint-enable no-duplicate-label */
(Assignee)

Comment 12

a year ago
Created attachment 8839860 [details] [diff] [review]
fix-1325987-1.patch

I think following aren't used
- browser_navigateEvents.js is not used (I put parse error, yet the test finsished successfully)
- browser_directorscript_actors.js
	(individually doesn't run, however ./mach test devtools/server/tests/browser/ runs fine)

- browser_register_actor.js
// Getting max 3 nested err if I use () => cleanupActor(actorFront)
// Hence I've retained the existing state
Attachment #8839331 - Attachment is obsolete: true
Attachment #8839860 - Flags: review?(ntim.bugs)
(Reporter)

Comment 13

a year ago
Comment on attachment 8839860 [details] [diff] [review]
fix-1325987-1.patch

Review of attachment 8839860 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good to me! Can you remove the directory from .eslintignore ?

jryans, can you look at the browser_navigateEvents.js changes? I'm not sure about them (even though I suggested them) since the test doesn't seem to work on my machine.

::: devtools/server/tests/browser/browser_canvasframe_helper_04.js
@@ +19,5 @@
>  
> +// const TEST_URL_1
> +//   = "data:text/html;charset=utf-8,CanvasFrameAnonymousContentHelper test 1";
> +const TEST_URL
> +  = "data:text/html;charset=utf-8,CanvasFrameAnonymousContentHelper test 2";

Looking again at this test. It seems like TEST_URL_1 being unused is a typo. So both variables should be kept, sorry I didn't look closely in my previous review.

It should probably be:
let browser = yield addTab(TEST_URL_1);

then:
content.location = TEST_URL_2;

@@ +68,5 @@
>  
>    info("Navigating to a new page");
>    let loaded = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +  // eslint-disable-next-line mozilla/no-cpows-in-tests
> +  content.location = TEST_URL;

You can avoid this CPOW by doing:

BrowserTestUtils.loadURI(browser, TEST_URL_2);

::: devtools/server/tests/browser/browser_directorscript_actors.js
@@ +49,4 @@
>    let { port } = yield installAndEnableDirectorScript(directorManager, {
>      scriptId: "testDirectorScript_MessagePort",
>      scriptCode: "(" + (function () {
> +      // eslint-disable-next-line no-shadow

You can remove this eslint-disable-next comment now.

::: devtools/server/tests/browser/browser_navigateEvents.js
@@ +146,5 @@
>        // Load another document in this doc to dispatch these events
>        assertEvent("load-new-document");
> +      ContentTask.spawn(gBrowser.selectedBrowser, {}, () => {
> +        contentcontent.location = URL2;
> +      });

There seems to be a typo (contentcontent).

Anyway, it's better to use:

gBrowser.loadURI(URL2);

or

BrowserTestUtils.loadURI(gBrowser.selectedBrowser, URL2);

instead of the ContentTask.spawn() block.
Attachment #8839860 - Flags: review?(ntim.bugs)
Attachment #8839860 - Flags: review+
Attachment #8839860 - Flags: feedback?(jryans)
(Reporter)

Comment 14

a year ago
(In reply to Ruturaj Vartak from comment #12)
> Created attachment 8839860 [details] [diff] [review]
> fix-1325987-1.patch
> 
> I think following aren't used
> - browser_navigateEvents.js is not used (I put parse error, yet the test
> finsished successfully)
> - browser_directorscript_actors.js
> 	(individually doesn't run, however ./mach test
> devtools/server/tests/browser/ runs fine)
Not sure about these two, maybe jryans can shed a light?

> - browser_register_actor.js
> // Getting max 3 nested err if I use () => cleanupActor(actorFront)
> // Hence I've retained the existing state
That whole test can be rewritten to use Task.async() to make it look more synchronous if you're up to the challenge. Though I would do that in another bug.
Comment on attachment 8839860 [details] [diff] [review]
fix-1325987-1.patch

Review of attachment 8839860 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at the browser.ini test manifest for this directory, `browser_navigateEvents.js` and several others only run when e10s is disabled.  If you run the entire directory, such a test will be skipped (since the default is e10s on) and if you run a test individually, the test always runs and manifest filters are ignored, but the test fails because e10s is on (the configuration where it is known to not work).

To run the test individually, you can use `./mach mochitest devtools/server/tests/browser/browser_navigateEvents.js --disable-e10s`.  The try server does still test the e10s off config, so we'll need to make the test still works in that mode at least.

The current changes for `browser_navigateEvents.js` plus :ntim's recent suggestions seem reasonable to me.

For `browser_directorscript_actors.js`, looks like it is also skipped for e10s on.  It seems like running with e10s off will cover all files in the directory, so maybe `./mach mochitest devtools/server/tests/browser --disable-e10s` is the best command for running these locally.
Attachment #8839860 - Flags: feedback?(jryans)
(Assignee)

Comment 16

a year ago
Created attachment 8840300 [details] [diff] [review]
fix-1325987-2.patch

- Thanks :jryans - your suggestions worked to execute the test
- Worked on :ntim's suggestions

I've executed the directory with `mochitest` as well `test`
====================
$ ./mach mochitest devtools/server/tests/browser/ --disable-e10s
...
TEST-INFO | checking window state
Browser Chrome Test Summary
	Passed: 1053
	Failed: 0
	Todo: 0
	Mode: non-e10s
*** End BrowserChrome Test Results ***
Buffered messages finished
SUITE-END | took 57s

$ ./mach test devtools/server/tests/browser/
...
TEST-INFO | checking window state
Browser Chrome Test Summary
	Passed: 827
	Failed: 0
	Todo: 0
	Mode: e10s
*** End BrowserChrome Test Results ***
Buffered messages finished
SUITE-END | took 49s
Attachment #8839860 - Attachment is obsolete: true
Attachment #8840300 - Flags: review?(ntim.bugs)
(Reporter)

Comment 17

a year ago
Comment on attachment 8840300 [details] [diff] [review]
fix-1325987-2.patch

Review of attachment 8840300 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks for the cleanup! Feel free to mark the next patch as r+ yourself.

::: .eslintignore
@@ +113,1 @@
>  !devtools/server/tests/browser/browser_webextension_inspected_window.js

Can you remove this line as well?

!devtools/server/tests/browser/browser_webextension_inspected_window.js
Attachment #8840300 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 18

a year ago
Created attachment 8840727 [details] [diff] [review]
fix-1325987-3.patch

Removing following line from .eslintignore

!devtools/server/tests/browser/browser_webextension_inspected_window.js
Attachment #8840300 - Attachment is obsolete: true
Attachment #8840727 - Flags: review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 19

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97694450aad
Fix ESLint issues in devtools/server/tests/browser/. r=ntim
Keywords: checkin-needed
(Assignee)

Comment 20

a year ago
Thanks Tim.

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d97694450aad
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.