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

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P5)

Description

[Junior [inactive]]

Thousand lines of constant chars make it hard to trace.

https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/netwerk/test/unit/test_authentication.js#670-1389

https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/netwerk/test/unit/test_authentication.js#1403-2122

Comment 1

[Diego Pino]

Attachment: Bug-1428243-Add-helper-method-to-build-a-payload.patch

The patch adds a helper method to build a large payload that can be used from both methods.

Comment 2

[Junior [inactive]]

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?

Comment 3

[Diego Pino]

Attachment: Bug-1428243-Add-helper-method-to-build-a-payload.patch

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).

Comment 4

[Junior [inactive]]

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

Comment 5

[Diego Pino]

Attachment: Bug-1428243-Add-helper-method-to-build-a-payload.-r-.patch

* 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

Comment 6

[Diego Pino]

Attachment: Add helper method to build a payload

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?

Comment 7

[Junior [inactive]]

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8ae180e0e4828725697c088113db9d0a3afd707

For the patch format, it's still not a 8 lines diff.

Comment 8

[Diego Pino]

Attachment: Bug-1428243-Add-helper-method-to-build-a-payload.-r-.patch

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

Comment 9

[Diego Pino]

: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?

Comment 10

[Junior [inactive]]

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.

Comment 11

[Diego Pino]

Thanks :juhsu. I didn't know needinfo? was meant for pinging too, thanks for the tip.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f22f793f7cd2
Add helper method to build a payload. r=juhsu

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/f22f793f7cd2

Comment 14

[Junior [inactive]]

(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