Define new test helper method - performRequests

RESOLVED FIXED in Firefox 61

Status

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: Honza, Assigned: shivanda.244610, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 61
good-first-bug
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

This is a follow up for bug 1419350
https://bugzilla.mozilla.org/show_bug.cgi?id=1419350#c3

Net monitor tests are often using the following construct:

let wait = waitForNetworkEvents(monitor, 2);
await ContentTask.spawn(tab.linkedBrowser, {}, () => {
  content.wrappedJSObject.performRequests(2);
});
await wait;

It would be great if we have a helper in head.js

async function performRequests(monitor, tab, count) {
  let wait = waitForNetworkEvents(monitor, count);
  await ContentTask.spawn(tab.linkedBrowser, count, requestCount => {
    content.wrappedJSObject.performRequests(requestCount);
  });
  await wait;
}

... and use it everywhere in tests that are using the construct.

Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 1

a year ago
Hey, may I work on this bug?
just to clear things: we have to change the code for all the files in 
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test
having the given construct. is that right?
Shivansh
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #1)
> Hey, may I work on this bug?
Sure!

> just to clear things: we have to change the code for all the files in 
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test
> having the given construct. is that right?
Correct

All the tests should use simply call:
`await performRequests(monitor, tab, <actual count>)`

Honza
Assignee: nobody → shivanda.244610
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
(Assignee)

Comment 3

a year ago
Hey, looking into the files I found that in some of the cases 'yield' has been used instead of 'await'. So do I also have to change the construct in which 'yield' has been used?(like this file: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_accessibility-01.js#33-37)
Secondly, some files already have 'performRequests' defined locally, so I believe I have to remove those local function also and call the function defined in the head.js file.
Thirdly, since I have to make changes in multiple files, how exactly do I have to submit these files(or patch)?
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #3)
> Hey, looking into the files I found that in some of the cases 'yield' has
> been used instead of 'await'. So do I also have to change the construct in
> which 'yield' has been used?(like this file:
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/
> browser_net_accessibility-01.js#33-37)
Keep the yield in place for now. We are replacing yield by await step by step
and it can be done as part of another report.

> Secondly, some files already have 'performRequests' defined locally, so I
> believe I have to remove those local function also and call the function
> defined in the head.js file.
Yes

> Thirdly, since I have to make changes in multiple files, how exactly do I
> have to submit these files(or patch)?
Just create a patch using:
hg diff > mypatch.patch

and attach the file here in the bug.

If you use HG queues you can:
hg push -f review
...to review board.

---

Some links

Read more about review board:
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

How to submit a patch
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Submitting review requests:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 5

a year ago
Posted patch mypatch.patch (obsolete) — Splinter Review
Helper function added in head.js, and added function call in the files that were using that function.
Attachment #8953114 - Flags: review+
(Assignee)

Comment 6

a year ago
Hey, I have attached the patch here. I was not able to push review as I am not able to add my IRC nick.

I have edited the 'head.js' file, and I found the constructor in only one file 'browser_net_view-source-debugger.js', so made the required changes in it.

Let me know if I am missing something or need to make some more changes in the patch.
(Assignee)

Comment 7

a year ago
Hey, can you please update me with progress of the bug.
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #5)
> Created attachment 8953114 [details] [diff] [review]
> mypatch.patch
> 
> Helper function added in head.js, and added function call in the files that
> were using that function.

You need to set the review to `?` to ask for a review.
The reviewer is consequently using `+` to indicate that the patch is ok or `-` if not

Honza
Attachment #8953114 - Flags: review+ → review?(odvarko)
Comment on attachment 8953114 [details] [diff] [review]
mypatch.patch

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

Thank for working on this!

Please see my inline comments.

Also, we want to use the new  helper in tests that are implementing own version of the function.

You can search the codebase (devtools/client/netmonitor/test) for `performRequests`
to find all such tests. And consequently manually check (in case when the test would
use different name)

Honza

::: devtools/client/netmonitor/test/browser_net_view-source-debugger.js
@@ +16,4 @@
>    let { document, store, windowRequire } = monitor.panelWin;
>    let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>    store.dispatch(Actions.batchEnable(false));
> +  

Remove whitespace characters on this line

@@ +16,5 @@
>    let { document, store, windowRequire } = monitor.panelWin;
>    let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>    store.dispatch(Actions.batchEnable(false));
> +  
> +  //helper function moved to head.js

There must be a space between `//` and the first letter
The first letter should be capitalized.

@@ +22,1 @@
>  

Remove whitespaces on this line.

::: devtools/client/netmonitor/test/head.js
@@ +758,5 @@
>    await onResponseContent;
>  }
> +
> +
> +async function performRequests(monitor, tab, count) {

A comment in front of the function would be helpful. Something like:


/**
 * Helper function for executing XHRs on a test page.
 * 
 * @param {Number} count Number of requests to be executed.
 */
Attachment #8953114 - Flags: review?(odvarko)
Flags: needinfo?(odvarko)
(Assignee)

Comment 10

a year ago
Hey,

> Also, we want to use the new  helper in tests that are implementing own
> version of the function.
> 
> You can search the codebase (devtools/client/netmonitor/test) for
> `performRequests`
> to find all such tests. And consequently manually check (in case when the
> test would
> use different name)

I am unable to find files other than the file "browser_net_view-source-debugger.js" having the said construct.
Even after using several keywords to search(using grep) I get the same file only.

Can you please help me to find other files?
Flags: needinfo?(odvarko)
When searching for `performRequests` in C:\src\mozilla.org\mozilla-central\devtools\client\netmonitor\test\

I am seeing usage in:
* browser_net_accessibility-01.js
* browser_net_accessibility-02.js
* browser_net_api-calls.js
* browser_net_api-calls.js (already has local helper)
* browser_net_brotli.js
* browser_net_cached-status.js (already has local helper)
* browser_net_cause_redirect.js (already has local helper)
* browser_net_complex-params.js:
* browser_net_content-type.js:
* browser_net_copy_image_as_data_uri.js:
* browser_net_copy_params.js:
* ... and many others

See my full search results here:
https://pastebin.com/gAJB40J0

Do you see it now?

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 12

a year ago
Hey, surely these files use performRequest but these don't use the construct given in the description.
Mostly, they are using the 'yield' instead of 'await', rest all other lines are same as that of the given construct.
So should I make a different helper for the construct which are using 'yield'?


I am not sure whether the HTML files having the performRequest function are concerned here or not, 
as they are not using the given construct.


And lastly, I believe that the helper 'performRequest' and "content.wrappedJSObject.'performRequests()'" 
are both different things, and we are only concerned about the helper function. Right?
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #12)
> Hey, surely these files use performRequest but these don't use the construct
> given in the description.
> Mostly, they are using the 'yield' instead of 'await', rest all other lines
> are same as that of the given construct.
> So should I make a different helper for the construct which are using
> 'yield'?
There should be just one helper and tests using yield should be adopted to use await.

An existing test:

add_task(function* () {
  // ...
  let wait = waitForNetworkEvents(monitor, 2);
  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {
    content.wrappedJSObject.performRequests(2);
  });
  yield wait;
  // ...
});

Should be converted into:

add_task(async () => {
  // ...
  await performRequests(monitor, 2);
  // ...
});

Btw. it's also preferred to use arrow-function

> I am not sure whether the HTML files having the performRequest function are
> concerned here or not, as they are not using the given construct.
Correct, the helper function isn't related to HTML files.

> And lastly, I believe that the helper 'performRequest' and
> "content.wrappedJSObject.'performRequests()'" 
> are both different things, and we are only concerned about the helper
> function. Right?
Yes

Honza
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #12)
> Mostly, they are using the 'yield' instead of 'await', rest all other lines
> are same as that of the given construct.

Good news is that bug 1440321 is replacing `yield` by `await`, so it'll make
work on this bug easier. Please wait for the patch/patches in bug 1440321
before finishing this bug.

Honza
Depends on: 1440321
Depends on: 1443470
(Assignee)

Comment 15

a year ago
(In reply to Jan Honza Odvarko [:Honza] from comment #14)
 
> Good news is that bug 1440321 is replacing `yield` by `await`, 

Finally, it is happening :P.
I guess we will have to wait now before we wrap this up.
(In reply to Shivansh from comment #15)
> I guess we will have to wait now before we wrap this up.
Bug 1443470 is fixed, so we can move forward with this one.
Honza
(Assignee)

Comment 17

a year ago
Posted patch mypatch2.patch (obsolete) — Splinter Review
Hey, I have fixed most of the files. Some of the files were calling content.wrappedJSObject.performRequests(args, url), i.e. with multiple arguments.
So if I could get some definition for the above function I can try to fix the remaining files also(if its needed).

Please let me know if the patch is missing something, other than the above mentioned.
Flags: needinfo?(odvarko)
Attachment #8957873 - Flags: review?(odvarko)
Great!

I tried to apply the patch, but I am seeing a lot of conflicts. Are you sure you are working with the latest m-c?


See some of them:
$ hg qpush
applying perform-requests.patch
patching file devtools/client/netmonitor/test/browser_net_accessibility-01.js
Hunk #1 FAILED at 29
Hunk #2 FAILED at 52
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_accessibility-01.js.rej
patching file devtools/client/netmonitor/test/browser_net_accessibility-02.js
Hunk #1 FAILED at 29
Hunk #2 FAILED at 54
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_accessibility-02.js.rej
patching file devtools/client/netmonitor/test/browser_net_api-calls.js
Hunk #1 FAILED at 28
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_api-calls.js.rej
patching file devtools/client/netmonitor/test/browser_net_brotli.js
Hunk #1 FAILED at 24
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_brotli.js.rej
patching file devtools/client/netmonitor/test/browser_net_cause_redirect.js


I was quickly looking at the new `performRequests` function. What is the `obj` argument for? It's not being used.

There are two patches attached at the moment. Should you mark the first one as obsolete?

Honza


Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 19

a year ago
Posted patch mypatch3.patch (obsolete) — Splinter Review
Attachment #8953114 - Attachment is obsolete: true
Attachment #8957873 - Attachment is obsolete: true
Attachment #8957873 - Flags: review?(odvarko)
Attachment #8958149 - Flags: review?(odvarko)
(Assignee)

Comment 20

a year ago
(In reply to Jan Honza Odvarko [:Honza] from comment #18)

> I tried to apply the patch, but I am seeing a lot of conflicts. Are you sure
> you are working with the latest m-c?
Apparently, I wasn't doing 'hg update' after 'hg pull'.


> I was quickly looking at the new `performRequests` function. What is the
> `obj` argument for? It's not being used.
I used another argument so that 'performRequest' can accept arguments for which, in place of, '{}' has an url,
or some other count. But for now, I have ignored these constructs.
 
> There are two patches attached at the moment. Should you mark the first one
> as obsolete?
Done!.


I have submitted another patch considering all the above things. Please let me know if I am still missing something.
Flags: needinfo?(odvarko)
Comment on attachment 8958149 [details] [diff] [review]
mypatch3.patch

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

Thanks fro the patch, looks great!

Just a few comments inline.

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +50,5 @@
>    store.dispatch(Actions.selectDelta(-10));
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_accessibility-02.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +52,5 @@
>    EventUtils.sendKey("HOME", window);
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_api-calls.js
@@ +29,5 @@
>      "http://example.com/api/search/?q=search%E2%98%A2"
>    ];
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 5)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_resend.js
@@ +25,5 @@
>  
>    store.dispatch(Actions.batchEnable(false));
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2);

This test is still using generator, so you need to use yield.

::: devtools/client/netmonitor/test/head.js
@@ +749,5 @@
> + * Helper function for executing XHRs on a test page.
> + *
> + * @param {Number} count Number of requests to be executed.
> + */
> +async function performRequests(monitor, tab, count) {

Please put the name of the function in the list of exported globals at the top of this file.
Attachment #8958149 - Flags: review?(odvarko)
Flags: needinfo?(odvarko)
Comment on attachment 8958149 [details] [diff] [review]
mypatch3.patch

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

Thanks fro the patch, looks great!

Just a few comments inline.

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +50,5 @@
>    store.dispatch(Actions.selectDelta(-10));
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_accessibility-02.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +52,5 @@
>    EventUtils.sendKey("HOME", window);
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_api-calls.js
@@ +29,5 @@
>      "http://example.com/api/search/?q=search%E2%98%A2"
>    ];
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 5)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
@@ +35,5 @@
>    info("Reloading the debuggee and performing all requests again...");
>    await triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> +  // Execute requests.
> +  // +1 extra document reload
> +  await performRequests(monitor, tab, IMAGE_TOOLTIP_REQUESTS + 1);

The refactoring isn't right, the test now fails on time out (never finishes).

You can run the test locally by:

./mach test devtools/client/netmonitor/test/browser_net_image-tooltip.js

::: devtools/client/netmonitor/test/browser_net_resend.js
@@ +25,5 @@
>  
>    store.dispatch(Actions.batchEnable(false));
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2);

This test is still using generator, so you need to use yield.

::: devtools/client/netmonitor/test/browser_net_send-beacon.js
@@ -19,5 @@
>    is(store.getState().requests.requests.size, 0, "The requests menu should be empty.");
>  
> -  let wait = waitForNetworkEvents(monitor, 1);
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function () {
> -    content.wrappedJSObject.performRequest();

The original name of the content method is 'performRequest'. The method is defined in html_send-beacon.html

It needs to be renamed to 'performRequests'

::: devtools/client/netmonitor/test/head.js
@@ +749,5 @@
> + * Helper function for executing XHRs on a test page.
> + *
> + * @param {Number} count Number of requests to be executed.
> + */
> +async function performRequests(monitor, tab, count) {

Please put the name of the function in the list of exported globals at the top of this file.
Comment on attachment 8958149 [details] [diff] [review]
mypatch3.patch

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

Thanks fro the patch, looks great!

Just a few comments inline.

Honza

::: devtools/client/netmonitor/test/browser_net_accessibility-01.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +50,5 @@
>    store.dispatch(Actions.selectDelta(-10));
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_accessibility-02.js
@@ +30,5 @@
>        "The network details panel should render correctly.");
>    }
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2)

missing semicolon

@@ +52,5 @@
>    EventUtils.sendKey("HOME", window);
>    check(0, true);
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 18)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_api-calls.js
@@ +29,5 @@
>      "http://example.com/api/search/?q=search%E2%98%A2"
>    ];
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 5)

missing semicolon

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
@@ +35,5 @@
>    info("Reloading the debuggee and performing all requests again...");
>    await triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> +  // Execute requests.
> +  // +1 extra document reload
> +  await performRequests(monitor, tab, IMAGE_TOOLTIP_REQUESTS + 1);

The refactoring isn't right, the test now fails on time out (never finishes).

You can run the test locally by:

./mach test devtools/client/netmonitor/test/browser_net_image-tooltip.js

::: devtools/client/netmonitor/test/browser_net_resend.js
@@ +25,5 @@
>  
>    store.dispatch(Actions.batchEnable(false));
>  
> +  // Execute requests.
> +  await performRequests(monitor, tab, 2);

This test is still using generator, so you need to use yield.

::: devtools/client/netmonitor/test/browser_net_send-beacon.js
@@ -19,5 @@
>    is(store.getState().requests.requests.size, 0, "The requests menu should be empty.");
>  
> -  let wait = waitForNetworkEvents(monitor, 1);
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function () {
> -    content.wrappedJSObject.performRequest();

The original name of the content method is 'performRequest'. The method is defined in html_send-beacon.html

It needs to be renamed to 'performRequests'

::: devtools/client/netmonitor/test/head.js
@@ +749,5 @@
> + * Helper function for executing XHRs on a test page.
> + *
> + * @param {Number} count Number of requests to be executed.
> + */
> +async function performRequests(monitor, tab, count) {

Please put the name of the function in the list of exported globals at the top of this file.
Sorry, I published the comments more times, pleas read the last one.

Honza
(Assignee)

Comment 25

a year ago
Posted patch mypatch4.patch (obsolete) — Splinter Review
Attachment #8958149 - Attachment is obsolete: true
Attachment #8958511 - Flags: review?(odvarko)
(Assignee)

Comment 26

a year ago
Hey, I've added all the required changes in the new patch.

I am not able to test the file. The error is as follows: 
"Exception: Binary expected at /home/shivansh/src/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox does not exist."
Even after several 'hg pull' and 'hg update', I am not able to get the binary.
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #26)
> Hey, I've added all the required changes in the new patch.
> 
> I am not able to test the file. The error is as follows: 
> "Exception: Binary expected at
> /home/shivansh/src/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox
> does not exist."
> Even after several 'hg pull' and 'hg update', I am not able to get the
> binary.
You need to build Firefox using:

./mach build


You can put:

ac_add_options --enable-artifact-builds

...into your .mozconfig file to make the build a lot faster.

Honza
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #26)
> Even after several 'hg pull' and 'hg update', I am not able to get the
> binary.

Btw. I am usually using the following command to update my source working directory:

hg pull -u

... which executes both 'hg pull' and 'hg update'


Honza
(Assignee)

Comment 29

a year ago
(In reply to Jan Honza Odvarko [:Honza] from comment #27)
> You need to build Firefox using:
> 
> ./mach build
Thanks, I am now able to test the files.

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> ::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
> @@ +35,5 @@
> >    info("Reloading the debuggee and performing all requests again...");
> >    await triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> > +  // Execute requests.
> > +  // +1 extra document reload
> > +  await performRequests(monitor, tab, IMAGE_TOOLTIP_REQUESTS + 1);
> 
> The refactoring isn't right, the test now fails on time out (never finishes).
> 
> You can run the test locally by:
> 
> ./mach test devtools/client/netmonitor/test/browser_net_image-tooltip.js
I am attaching a working patch for this file. Please review it.
Flags: needinfo?(odvarko)
(Assignee)

Comment 30

a year ago
Posted patch br_net_img_ttip.patch (obsolete) — Splinter Review
Attachment #8958902 - Flags: review?(odvarko)
Here is try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6874ae7cf779413adad19df38895afe9c2f2872a&selectedJob=168150118

I am still seeing some tests to fail.
There are also some eslint errors that needs to be fixed.

Mainly: "Unexpected space before function parentheses." (it's new rule introduced recently, so that's why it appeared now)

Also, please merge both patches together, thanks!

Honza
Flags: needinfo?(odvarko)
Comment on attachment 8958511 [details] [diff] [review]
mypatch4.patch

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

Unassigning myself from the review for now till mentioned issues are fixed.
Attachment #8958511 - Flags: review?(odvarko)
Comment on attachment 8958902 [details] [diff] [review]
br_net_img_ttip.patch

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

Unassigning myself from the review for now till mentioned issues are fixed.

Honza
Attachment #8958902 - Flags: review?(odvarko)
(Assignee)

Comment 34

a year ago
Posted patch mypatch5.patch (obsolete) — Splinter Review
Attachment #8958511 - Attachment is obsolete: true
Attachment #8958902 - Attachment is obsolete: true
Attachment #8959774 - Flags: review?(odvarko)
Comment on attachment 8959774 [details] [diff] [review]
mypatch5.patch

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

Excellent, looks good, thanks for working on this!

R+

Try push looks ok
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9dc2373389f11bf66ec4bff8d007f5b2e816fe5

Honza
Attachment #8959774 - Flags: review?(odvarko) → review+
(Assignee)

Comment 36

a year ago
Hey, happy to know that my patch is acceptable. So, is there something still left to be done? or the bug has been resolved?
Flags: needinfo?(odvarko)
(In reply to Shivansh from comment #36)
> Hey, happy to know that my patch is acceptable. So, is there something still
> left to be done? or the bug has been resolved?

The next step is to set a flag checkin-needed into the Keyword field (it's the one with good-first-bug flag, you need to separate them by a comma). This way, sheriffs are informed about pending patches and can commit them. 

Contributors with commit access can commit patches with R+ manually.

So, please set the flag and wait till somebody lands this into m-c repo.

Thanks for the help!

Honza
Flags: needinfo?(odvarko)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
This patch failed to apply. Please take a look.

applying mypatch5.patch
patching file devtools/client/netmonitor/test/browser_net_params_sorted.js
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_params_sorted.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh mypatch5.patch
Flags: needinfo?(shivanda.244610)
Keywords: checkin-needed
(Assignee)

Comment 39

a year ago
Hey, this should solve the above problem.
Attachment #8959774 - Attachment is obsolete: true
Flags: needinfo?(shivanda.244610)
Attachment #8961348 - Flags: review?(odvarko)
Comment on attachment 8961348 [details] [diff] [review]
mypatch6.patch

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

Works for me.

R+

Thanks!
Honza
Attachment #8961348 - Flags: review?(odvarko) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 41

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2247b1e7324f
Define new performRequests test helper method. r=Honza
Keywords: checkin-needed

Comment 42

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2247b1e7324f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.