744387 - SimpleTest._logResult() should report when test.name is 'undefined'

Status: RESOLVED WONTFIX

(Testing :: Mochitest, defect)

Description

[Serge Gautherie (:sgautherie)]

This would make logs less suspicious (and possibly catch coding mistakes).

Ted, do you agree?

***

{
269 SimpleTest._logResult = function(test, passString, failString) {

273     var diagnostic = test.name + (test.diag ? " - " + test.diag : "");
}

The additional check should be a todo() while tests are fixed, then an ok().

Comment 1

[Martijn Wargers (dead)]

Attachment: 744387.diff

This seems like a good idea.
We should first fix the existing wrong callers, though.
I pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=7cbc50ae5455

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #1)

> Created attachment 8473741 [details] [diff] [review]
> 744387.diff
> 
> This seems like a good idea.

(Thanks, for taking this bug.)

I assume fixing this bug in 'SimpleTest._logResult()' would be too late because the outside 'test' variable should be kept in sync' with logs.
Is that it?

{{
360    // BUGFIX : coercing test.name to a string, because some a11y tests pass an xpconnect object
}}
While here, that bugfix should probably be moved/merged (then fixed).

In addition, (at least)
{{
SimpleTest.info = function(name, message) {
}}
would probably want same check too.

> We should first fix the existing wrong callers, though.

Maybe this is not required/wanted if we can do something like
{{
 SimpleTest.ok = function (condition, name, diag) {
+    if (!name) {
+      todo(false, name, diag + 'code to record the initial condition value');
+      return;
+    }
}}
in the meantime.

Though my initial idea would have been something like
{{
+    if (!name) {
+      todo(false, 'No description given for subtest, please fix', diag + 'code to record the initial condition value');
+      // No return.
+    }
}}
everywhere.

> I pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=7cbc50ae5455

Ftr, https://hg.mozilla.org/try/rev/6791bfaf77bd

Comment 3

[Martijn Wargers (dead)]

(In reply to Serge Gautherie (:sgautherie) from comment #2)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #1)
> 
> > Created attachment 8473741 [details] [diff] [review]
> > 744387.diff
> > 
> > This seems like a good idea.
> 
> (Thanks, for taking this bug.)
> 
> I assume fixing this bug in 'SimpleTest._logResult()' would be too late
> because the outside 'test' variable should be kept in sync' with logs.
> Is that it?

If I would put the logic in SimpleTest._logResult(), then in a single testrun, the "No description given for subtest, please fix" wouldn't be picked up, it would still give "undefined".
Otherwise, it wouldn't matter, I think.

For the rest, I first want to see the existing callers fixed.
In my opinion, if test authors are misusing the SimpleTest.is/ok/etc methods, this should generate an error.

Tryservers shows these tests that need fixing:
content/base/test/test_XHRSendData.html
content/base/test/test_XHR_anon.html
content/base/test/test_XHR_system.html
content/base/test/test_bug1025933.html
content/base/test/test_bug166235.html
content/base/test/test_bug357450.html
content/base/test/test_bug357450.xhtml
content/base/test/test_bug357450_svg.xhtml
content/base/test/test_bug364413.xhtml
content/base/test/test_bug382871.html
content/base/test/test_bug409380.html
content/base/test/test_bug422403-1.html
content/base/test/test_bug424359-1.html
content/base/test/test_bug562137.html
content/base/test/test_bug564047.html
content/base/test/test_bug567350.html
content/base/test/test_bug585978.html (horrible!)
content/base/test/test_bug638112.html
content/base/test/test_bug744830.html
content/base/test/test_bug761120.html
content/base/test/test_copypaste.html
content/base/test/test_createHTMLDocument.html
content/base/test/test_file_from_blob.html
content/base/test/test_ipc_messagemanager_blob.html
content/base/test/test_mutationobservers.html
content/base/test/test_websocket.html
content/test/forms/test_input_attributes_reflection.html
content/html/content/test/forms/test_input_color_input_change_events.html
content/html/content/test/forms/test_input_event.html
content/html/content/test/forms/test_input_number_mouse_events.html
content/html/content/test/forms/test_submit_invalid_file.html
content/html/content/test/forms/test_valueasdate_attribute.html
content/html/content/test/test_a_text.html
content/html/content/test/test_anchor_href_cache_invalidation.html
content/html/content/test/test_bug386728.html
content/html/content/test/test_bug386996.html
content/html/content/test/test_bug421640.html
content/html/content/test/test_bug579079.html
content/html/content/test/test_bug589.html
content/html/content/test/test_bug615595.html
content/html/content/test/test_bug641219.html
content/html/content/test/test_hidden.html
content/html/content/test/test_object_plugin_nav.html
content/html/document/test/test_bug446483.html
content/html/document/test/test_bug499092.html
content/media/test/test_bug919265.html
content/media/test/test_bug957847.html
content/media/test/test_can_play_type.html
content/media/test/test_constants.html
content/media/test/test_currentTime.html
content/media/test/test_readyState.html
content/media/test/test_texttrackcue.html
content/media/webspeech/recognition/test/test_call_start_from_end_handler.html
content/svg/content/test/test_SVGStyleElement.xhtml
content/svg/content/test/test_animLengthReadonly.xhtml
content/svg/content/test/test_pointAtLength.xhtml
content/svg/content/test/test_text_dirty.html
content/svg/content/test/test_text_lengthAdjust.html
content/svg/content/test/test_text_selection.html
docshell/test/test_bug475636.html
docshell/test/test_bug590573.html
docshell/test/test_bug660404.html
dom/base/test/test_navigator_language.html
dom/base/test/test_url.html
dom/base/test/test_urlSearchParams.html

dom/browser-element/mochitest/test_browserElement_inproc_Alert.html
dom/browser-element/mochitest/test_browserElement_inproc_AlertInFrame.html
dom/browser-element/mochitest/test_browserElement_inproc_BackForward.html
dom/browser-element/mochitest/test_browserElement_inproc_DOMRequestError.html
dom/browser-element/mochitest/test_browserElement_inproc_Metachange.html
dom/browser-element/mochitest/test_browserElement_inproc_OpenNamed.html
dom/browser-element/mochitest/test_browserElement_inproc_OpenWindowInFrame.html
dom/browser-element/mochitest/test_browserElement_inproc_OpenWindowRejected.html
dom/browser-element/mochitest/test_browserElement_inproc_Opensearch.html
dom/browser-element/mochitest/test_browserElement_inproc_PromptCheck.html
dom/browser-element/mochitest/test_browserElement_inproc_ReloadPostRequest.html
dom/browser-element/mochitest/test_browserElement_inproc_SecurityChange.html
dom/browser-element/mochitest/test_browserElement_inproc_SetVisibleFrames.html
dom/browser-element/mochitest/test_browserElement_inproc_TargetBlank.html
dom/browser-element/mochitest/test_browserElement_inproc_Titlechange.html
dom/browser-element/mochitest/test_browserElement_oop_Alert.html
dom/browser-element/mochitest/test_browserElement_oop_AlertInFrame.html
dom/browser-element/mochitest/test_browserElement_oop_BackForward.html
dom/browser-element/mochitest/test_browserElement_oop_DOMRequestError.html
dom/browser-element/mochitest/test_browserElement_oop_Iconchange.html
dom/browser-element/mochitest/test_browserElement_oop_Metachange.html
dom/browser-element/mochitest/test_browserElement_oop_OpenNamed.html
dom/browser-element/mochitest/test_browserElement_oop_OpenWindow.html
dom/browser-element/mochitest/test_browserElement_oop_OpenWindowInFrame.html
dom/browser-element/mochitest/test_browserElement_oop_OpenWindowRejected.html
dom/browser-element/mochitest/test_browserElement_oop_Opensearch.html
dom/browser-element/mochitest/test_browserElement_oop_PromptCheck.html
dom/browser-element/mochitest/test_browserElement_oop_ReloadPostRequest.html
dom/browser-element/mochitest/test_browserElement_oop_RemoveBrowserElement.html
dom/browser-element/mochitest/test_browserElement_oop_SecurityChange.html
dom/browser-element/mochitest/test_browserElement_oop_SetVisibleFrames.html
dom/browser-element/mochitest/test_browserElement_oop_Titlechange.html
dom/canvas/test/test_2d.isPointInPath.winding.html
dom/events/test/test_bug368835.html
dom/events/test/test_bug379120.html
dom/events/test/test_bug448602.html
dom/events/test/test_bug615597.html
dom/events/test/test_bug944847.html
dom/events/test/test_bug998809.html
dom/events/test/test_eventctors.html

And those are only the first 2 chunks of mochitest runs.
Perhaps developers don't care much of adding a descriptive comment for subtests.
Probably better to find that out first, before working on this bug.

Comment 4

[Martijn Wargers (dead)]

Ted, would it be useful to change this or should test authors just be able to write stuff like ok(true), is(a,b), etc?

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> Ted, would it be useful to change this or should test authors just be able
> to write stuff like ok(true), is(a,b), etc?

Given your huge list of tests in comment 3 it doesn't seem very worthwhile to enforce this. We already have a test name, maybe we can add a line number into the output to make assertions clearer? (I've wanted that even for assertions with a message, it's not always clear which one they are.)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

:mwargers, you are assigned as a mentor here, is there documented work for someone to do that you could mentor?

Comment 7

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #6)
> :mwargers, you are assigned as a mentor here, is there documented work for
> someone to do that you could mentor?

Joel, it is pretty self-explanatory, no? But Ted thinks it's of limited value, so perhaps better to just WONTFIX this, because it is a lot of work with little value.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
>  maybe we can add a line number
> into the output to make assertions clearer? (I've wanted that even for
> assertions with a message, it's not always clear which one they are.)

Good idea, I filed bug 1101039 for it.

Comment 8

[Prakhar Agrawal [:prakhar0409]]

Hi, is the WONTFIX in the comment above still valid? I am trying to get started. Could someone help me out on how to setup environment to get these things up right and working?

Comment 9

[Martijn Wargers (dead)]

(In reply to Prakhar Agrawal [:prakhar0409] from comment #8)
> Hi, is the WONTFIX in the comment above still valid? I am trying to get
> started. Could someone help me out on how to setup environment to get these
> things up right and working?

Hi Prakhar, this bug is very likely wontfix, especially now that bug 1101039 is fixed.