Closed Bug 1428243 Opened 2 years ago Closed Last year

Shorten the tedious large realm in netwerk/test/unit/test_authentication.js

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: junior, Assigned: dpino)

Details

(Keywords: good-first-bug, Whiteboard: [necko-triaged])

Attachments

(1 file, 4 obsolete files)

The patch adds a helper method to build a large payload that can be used from both methods.
Attachment #9001539 - Flags: review?(juhsu)
Comment on attachment 9001539 [details] [diff] [review]
Bug-1428243-Add-helper-method-to-build-a-payload.patch

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

Thanks for your patch!

The patch format needs some changes, such as 8 lines diff
|./mach bootstrap| or |./mach mercurial-setup| could help.

::: netwerk/test/unit/test_authentication.js
@@ +686,5 @@
> +    let ret = [];
> +    for (let i = 0; i < size; i++) {
> +        ret[i] = 'a';
> +    }
> +    return ret.join();

I'm not a js export.
However, str+ should be faster than Array.join().
Correct me if I'm wrong

@@ +687,5 @@
> +    for (let i = 0; i < size; i++) {
> +        ret[i] = 'a';
> +    }
> +    return ret.join();
> +}

nit: two-spaces indentation

@@ +694,2 @@
>    var body;
> +  var size = 33*1024; // test > 32KB realm tokens

nit: space around '*'

@@ +706,2 @@
>    var body;
> +  var size = 33*1024; // test > 32KB domain tokens

same here

@@ +708,4 @@
>  
>    response.setStatusLine(metadata.httpVersion, 401, "Unauthorized");
>    response.setHeader("WWW-Authenticate",
> +		     'Digest realm="foo", domain="' + buildPayload(size) + '"');

Does it good to cache and reuse the result?
Attachment #9001539 - Flags: review?(juhsu)
Thanks for the review. I pushed a new patch addressing the changes requested. As for performance of string concatenation, you were right, + is preferred to join() (https://stackoverflow.com/questions/16696632/most-efficient-way-to-concatenate-strings-in-javascript).
Attachment #9001539 - Attachment is obsolete: true
Attachment #9003057 - Flags: review?(juhsu)
Comment on attachment 9003057 [details] [diff] [review]
Bug-1428243-Add-helper-method-to-build-a-payload.patch

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

r=me. Please use [1] to create a patch with the correct format

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3F

::: netwerk/test/unit/test_authentication.js
@@ +687,5 @@
> +  let ret = "";
> +  return function() {
> +    if (ret.length > 0) {
> +      return ret;
> +    }

annotate the intention of caching
Attachment #9003057 - Flags: review?(juhsu) → review+
* Added comment about cached value.
* Added reviewer to commit message (is it r=me correct? I don't even have committer status)

As for the patch format, I'm currently using a git checkout. I use Mozilla's git tools [1] to convert a git patch to an hg patch (git-patch-to-hg-patch). However, in the previous patch I didn't generate the original git patch with the right format, so that might be the issue. This time I used "git format-patch -k -p" as commented in [2]

[1] https://github.com/jlebar/moz-git-tools
[2] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #9003057 - Attachment is obsolete: true
I amended the patch to include r=juhsu (sorry for the misunderstanding). Is the format of the patch correct now?

I would need to run the try-server, but I don't have commit access. Could you help me with that?
Attachment #9003426 - Attachment is obsolete: true
try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8ae180e0e4828725697c088113db9d0a3afd707

For the patch format, it's still not a 8 lines diff.
Assignee: nobody → dpino
I managed to generate the patch with 8 lines of context.

It was a bit tricky. I'm using a git checkout with Mozilla's Git tools to convert a git patch to hg format (git-patch-to-hg-patch). The docs say the git patch should be generated like this [1]:

$ git format-patch -k -p

But that leaves the patch with the diff's lines of context default value (usually 3 lines). To force the patch to be a 8 lines diff patch, it should be like this:

$ git format-patch -k -p -U8

Or perhaps it would be convenient to add a comment in that section indicating patches must have 8 lines of context and leave instructions on how to set that in Git (I believe there must be a default config option).

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial
Attachment #9005985 - Attachment is obsolete: true
:juhsu: If the patch format is correct now, I'd like to add the "checkin-needed" tag so the patch can land. Could you confirm the patch format is correct?
The format is good now.

Here's the try, looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8ae180e0e4828725697c088113db9d0a3afd707

Next time you can use needinfo? so that I won't miss your question.
Thanks :juhsu. I didn't know needinfo? was meant for pinging too, thanks for the tip.
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f22f793f7cd2
Add helper method to build a payload. r=juhsu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f22f793f7cd2
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(In reply to Diego Pino from comment #11)
> Thanks :juhsu. I didn't know needinfo? was meant for pinging too, thanks for
> the tip.

For me, most of bug mails are filtered under a folder/label.
needinfo flag is an exception. Those mails are directly dispatched to my inbox. 

Please see [1] for more information
[1] https://wiki.mozilla.org/BMO/How_to_Use_Bugzilla#needinfo_flag
You need to log in before you can comment on or make changes to this bug.