Closed Bug 468024 Opened 11 years ago Closed 11 years ago

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

Categories

(Testing :: Reftest, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sgautherie, Assigned: sgautherie)

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1rc3+])

Attachments

(1 file, 1 obsolete file)

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 [...].
}}
Component: Layout: Misc Code → Reftest
Product: Core → Testing
Version: Trunk → unspecified
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #377235 - Flags: review?(dbaron)
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.
Attachment #377235 - Flags: review?(dbaron) → review-
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.
(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...)
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).
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.
Ah, right... then I'd prefer leaving it as it is.
Av1, with comment 7 suggestion(s).
Attachment #377235 - Attachment is obsolete: true
Attachment #377324 - Flags: review?(dbaron)
Comment on attachment 377324 [details] [diff] [review]
(Av2) Check for no tests case, Whitespace cleanups
[Checkin: See comment 10 & 11]

r=dbaron
Attachment #377324 - Flags: review?(dbaron) → review+
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
Attachment #377324 - Attachment description: (Av2) Check for no tests case, Whitespace cleanups → (Av2) Check for no tests case, Whitespace cleanups [Checkin: Comment 10]
Severity: trivial → enhancement
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Whiteboard: [needs 1.9.1 landing: depends on bug 396226]
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
}
Attachment #377324 - Attachment description: (Av2) Check for no tests case, Whitespace cleanups [Checkin: Comment 10] → (Av2) Check for no tests case, Whitespace cleanups [Checkin: See comment 10 & 11]
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing: depends on bug 396226] → [fixed1.9.1rc3+]
You need to log in before you can comment on or make changes to this bug.