test_bug351601.html does not throw, but does not test either

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: ray, Assigned: ray)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
This is a very simple test case. The bug says that, at one time, if the build ID was not set, then reading the navigator.buildID ivar from JavaScript would cause a crash.

So, this test verifies that this call cannot crash.

But this test, being part of the automated harness, will run only in environments where the build ID is set. There are no plans, AFAIK, to regularly build an app that does not have a build ID and then run the test suite on it.

So, this test should check. If the build ID is set and this call does not throw, it should call todo(). It calls ok() right now. It should only do that if there build ID is not set.

To claim that the test passes when the build ID is set is a false positive result.
(Assignee)

Comment 1

12 years ago
Created attachment 257556 [details] [diff] [review]
patch to fix bug, todo if in production build

Comment 2

12 years ago
Did you forget to ask for review?

I don't understand why you make this todo(), it looks like it just doesn't need to be tested in production builds.
(Assignee)

Comment 3

12 years ago
Created attachment 258811 [details] [diff] [review]
updated, turning the todo() into an innocuous ok()


I changed the todo() to an innocuous ok(). When the test is run on production builds, it definitely should pass.
Attachment #257556 - Attachment is obsolete: true
Attachment #258811 - Flags: review?

Comment 4

12 years ago
Set the r? to sayrer. And I think setting isOK to false in the catch block and using the ternary operator instead of the if would be slightly better.
(Assignee)

Updated

12 years ago
Attachment #258811 - Flags: review? → review?(sayrer)

Updated

12 years ago
Attachment #258811 - Flags: review?(sayrer) → review+
(Assignee)

Updated

12 years ago
Whiteboard: [needs checkin]
Whiteboard: [needs checkin] → [checkin needed]

Updated

12 years ago
Assignee: nobody → ray
Flags: in-testsuite-

Comment 5

12 years ago
Checking in test_bug351601.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug351601.html,v  <--  test_bug351601.html
new revision: 1.2; previous revision: 1.1
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Component: Testing → DOM
QA Contact: testing → general
You need to log in before you can comment on or make changes to this bug.