468024 - reftest.js : optimize |outputs| assignment(s)

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Serge Gautherie (:sgautherie)]

Bug 466372 comment 7:
{{
From  Serge Gautherie   2008-12-01 17:45:18 PST

Shouldn't
{
            var outputs = {};
            const randomMsg = "(EXPECTED RANDOM)";
            ...
}
be global to the file rather than local to the function ?
(They are invariant and I guess that should improve/speed things up a
little...)
}}

Bug 466372 comment 8:
{{
From  David Baron [:dbaron]   2008-12-01 17:51:34 PST

No.  They're not needed outside the function.  |outputs| could probably be
written as a const rather than a var, and in a single expression, though [...].
}}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Check for no tests case, Whitespace cleanups, Optimize |outputs|

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 377235 [details] [diff] [review]
(Av1) Check for no tests case, Whitespace cleanups, Optimize |outputs|

I prefer the outputs assignment as it is, since it means that the constants can be changed.

I also don't see why you want the throw if !gTotalTests.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, you can initialize outputs as a single expression if you make it an object rather than an array.  Then it's safe to changes in the constants, and there's still no need to change the way it's accessed.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

> I also don't see why you want the throw if !gTotalTests.

Mochitests have this check:
browser-chrome in js, currently as 'TEST-PASS',
others in buildbot, as 'WARNINGS'.

I would want to synchronize all the tests suites,
and I thought the error/orange was the best way to notice this situation.
(As I don't think it would bother much anyone testing locally with no tests on purpose...)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

ok.

So I guess this should be ok if you switch from an array to an object so you can explicitly associate values with their indices (unless there's a way to do that with an array).

Comment 6

[Serge Gautherie (:sgautherie)]

I think I don't know the syntax to achieve that:
|const outputs = { EXPECTED_PASS : ...| would need to be accessed by |outputs["EXPECTED_PASS"]...|;
|const outputs = { 0 : ...| works but makes it even less clear that it depends on the EXPECTED_PASS value.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, right... then I'd prefer leaving it as it is.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Check for no tests case, Whitespace cleanups [Checkin: See comment 10 & 11]

Av1, with comment 7 suggestion(s).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 377324 [details] [diff] [review]
(Av2) Check for no tests case, Whitespace cleanups
[Checkin: See comment 10 & 11]

r=dbaron

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 377324 [details] [diff] [review]
(Av2) Check for no tests case, Whitespace cleanups
[Checkin: See comment 10 & 11]


http://hg.mozilla.org/mozilla-central/rev/f4d22a94a40e

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 377324 [details] [diff] [review]
(Av2) Check for no tests case, Whitespace cleanups
[Checkin: See comment 10 & 11]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e44f8e3209a5

after fixing context for
{
patching file layout/tools/reftest/reftest.js
Hunk #1 FAILED at 190
Hunk #2 FAILED at 243
Hunk #3 FAILED at 482
Hunk #4 FAILED at 553
4 out of 5 hunks FAILED
}