Make mochitest-browser tests provide a useful message for tests with neither add_task nor test() methods

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Gijs, Assigned: AMoz, Mentored)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

No description provided.
This would involve fixing up this:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js?rev=8f49394f8069#621

if else block to include a check that this.currentTest.scope.test is actually a function before trying to run it.

It should then get a final else block to throw an error with a user-friendly message indicating that the test has neither an add_task call nor a test() function.
Mentor: gijskruitbosch+bugs
Whiteboard: [good first bug][lang=js]
hello am interested to solve this but am newer can u guide me please???
gijs, checking in here to ensure this bug is still valid with the current tests and codebase.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Joel Maher (:jmaher) from comment #3)
> gijs, checking in here to ensure this bug is still valid with the current
> tests and codebase.

Yes.

16 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_stupidmochi.js | Exception thrown - at chrome://mochikit/content/browser-test.js:696 - TypeError: this.currentTest.scope.test is not a function


with a near-empty test js file added to browser/base/content/test/general/browser.ini .

Prasanna, I'm sorry I never got back to you. If you're still interested, please let us know.
Flags: needinfo?(gijskruitbosch+bugs)
Hey Gijs, I am interested in taking this bug. Please explain a little so that I can understand that what I need to do.
(In reply to deshrajdry from comment #5)
> Hey Gijs, I am interested in taking this bug. Please explain a little so
> that I can understand that what I need to do.

Hi,

comment #1 has most of the detail here:

(In reply to :Gijs Kruitbosch from comment #1)
> This would involve fixing up this:
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.
> js?rev=8f49394f8069#621
> 
> if else block to include a check that this.currentTest.scope.test is
> actually a function before trying to run it.
> 
> It should then get a final else block to throw an error with a user-friendly
> message indicating that the test has neither an add_task call nor a test()
> function.

A more current and slightly more detailed link is:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js?mark=712-714&rev=24c4bac4ad05#678

This else block should be:

else if (typeof this.currentTest.scope.test == "function") {
  this.currentTest.scope.test();
}

and then there should be another

... else {
  throw "This test didn't call add_task, nor did it define a generatorTest() function, nor did it define a test() function, so we don't know how to run it.";
}

You should test the current behaviour and then the fixed behaviour by creating an empty test file e.g. at:

browser/base/content/test/general/browser_removeme_fake_test.js

and adding a line with

[browser_removeme_fake_test.js]

at the bottom of

browser/base/content/test/general/browser.ini

and then running:

./mach mochitest-browser browser/base/content/test/general/browser_removeme_fake_test.js


The extra test shouldn't be a part of your patch.

Does that help clarify things? If not, please ask more detailed questions about which parts are not clear yet.
Hi Gijs, please share your irc nickname. I have understood everything except that before creating the file browser/base/content/test/general/browser_removeme_fake_test.js, I need to test the current behavior, so I need to run which command in order to check the current behavior. I have done rest of the things. :)
Posted patch bug1061813.patch (obsolete) — Splinter Review
ok I figured it out how to run the tests. :)
Comment on attachment 8570171 [details] [diff] [review]
bug1061813.patch

Review of attachment 8570171 [details] [diff] [review]:
-----------------------------------------------------------------

Great! This patch looks good, but there are a few minor issues that I identified below.

Can you upload a new version of the patch with those fix, and set the "review" flag on the attachment to "?" and fill in ":jmaher" (without quotes) in the textfield that appears next to it then? Thanks!

::: testing/mochitest/browser-test.js
@@ +707,5 @@
>          // This test is a generator. It will not finish immediately.
>          this.currentTest.scope.waitForExplicitFinish();
>          var result = this.currentTest.scope.generatorTest();
>          this.currentTest.scope.__generator = result;
> +        result.next();        

There's some whitespace at the end of this line now that shouldn't be there.

@@ +709,5 @@
>          var result = this.currentTest.scope.generatorTest();
>          this.currentTest.scope.__generator = result;
> +        result.next();        
> +      } else if (typeof this.currentTest.scope.test == "function") {
> +          this.currentTest.scope.test();

This should be indented two spaces compared to the previous line.

@@ +712,5 @@
> +      } else if (typeof this.currentTest.scope.test == "function") {
> +          this.currentTest.scope.test();
> +      }
> +      else {
> +          throw "This test didn't call add_task, nor did it define a generatorTest() function, nor did it define a test() function, so we don't know how to run it.";

And this, too.
Attachment #8570171 - Flags: feedback+
Posted patch bug1061813(revised).patch (obsolete) — Splinter Review
Attachment #8570171 - Attachment is obsolete: true
Attachment #8570625 - Flags: review?(jmaher)
Comment on attachment 8570625 [details] [diff] [review]
bug1061813(revised).patch

Review of attachment 8570625 [details] [diff] [review]:
-----------------------------------------------------------------

how did the test go that Gijs recommended in comment 6?

::: testing/mochitest/browser-test.js
@@ +711,5 @@
>          result.next();
> +        } else if (typeof this.currentTest.scope.test == "function") {
> +            this.currentTest.scope.test();
> +        } else {
> +            throw "This test didn't call add_task, nor did it define a generatorTest() function, nor did it define a test() function, so we don't know how to run it.";

nit: we have 2 space indentation in this file, please ensure your changes are only indented two space (I see 4 space).
Attachment #8570625 - Flags: review?(jmaher) → review-
hi deshrajdry,
Would you please confirm your interest in proceeding further for this bug ? So that I can look for another bug to work on. :)
Amod, seems deshrajdry hasn't been active on bugzilla for the last 3 months or so; would you like to take the attached patch further?
Flags: needinfo?(amod.narvekar)
Hi Gijs. Yes. I would request you to allow me start afresh instead of taking deshrajdry's patch ahead.
Flags: needinfo?(amod.narvekar)
(In reply to Amod [:AMoz] from comment #15)
> Hi Gijs. Yes. I would request you to allow me start afresh instead of taking
> deshrajdry's patch ahead.

I don't mind either way, as long as you fix the bug. :-)
(In reply to :Gijs Kruitbosch from comment #16)

> I don't mind either way, as long as you fix the bug. :-)

Thank you. I will upload the first patch within a day or two.
Posted patch patchv1 (obsolete) — Splinter Review
the test is throwing the exception as expected. 

>*** End BrowserChrome Test Results ***
>The following tests failed:
>13 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_removeme_fake_test.js | Exception thrown - at chrome://mochikit/content/browser-test.js:770 - TypeError: this.currentTest.scope.test is not a function
SUITE-END | took 43s

I am returning to bugzilla afer quite a long time. So, apologies in advance in case of mistakes on my side.
Attachment #8570625 - Attachment is obsolete: true
Attachment #8617142 - Flags: feedback?(jmaher)
Comment on attachment 8617142 [details] [diff] [review]
patchv1

(In reply to Amod [:AMoz] from comment #18)
> Created attachment 8617142 [details] [diff] [review]
> patchv1
> 
> the test is throwing the exception as expected. 
> 
> >*** End BrowserChrome Test Results ***
> >The following tests failed:
> >13 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_removeme_fake_test.js | Exception thrown - at chrome://mochikit/content/browser-test.js:770 - TypeError: this.currentTest.scope.test is not a function
> SUITE-END | took 43s
> 
> I am returning to bugzilla afer quite a long time. So, apologies in advance
> in case of mistakes on my side.

But we want it to not throw a type error, but throw the message you added. So it's not doing the right thing.

Did you rebuild the testing components with ./mach build testing/mochitest before trying to run the test?
Attachment #8617142 - Flags: feedback?(jmaher)
I am sorry I missed out that.

Now the test seem to give a desired message

>*** End BrowserChrome Test Results ***
>The following tests failed:
>13 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_removeme_fake_test.js | >Exception thrown - This test didn't call add_task, nor did it define a generatorTest() function, nor >did it define a test() function, so we don't know how to run it.
>SUITE-END | took 30s
hi Gijs,
Anything I need to modify further ?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Amod [:AMoz] from comment #21)
> hi Gijs,
> Anything I need to modify further ?

Ah, sorry for not getting back to you before. You should adjust the indentation of your changes to be 2-space instead of 4-space, and for the next revision of the patch, request review from :jmaher. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Posted patch patchv2 (obsolete) — Splinter Review
Removed extra whitespaces as suggested. 
And also, self assigning the bug so as to remove it from list of "bugs with no owners" at bugsahoy.
Assignee: nobody → amod.narvekar
Attachment #8617142 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8621074 - Flags: review?
Comment on attachment 8621074 [details] [diff] [review]
patchv2

Review of attachment 8621074 [details] [diff] [review]:
-----------------------------------------------------------------

Please see some feedback below. When you next attach a patch, fill in ":jmaher" (without quotes) in the box next to the review flag dropdown.

::: browser/base/content/test/general/browser.ini
@@ +479,5 @@
>  [browser_bug1124271_readerModePinnedTab.js]
>  support-files =
>    readerModeArticle.html
>  [browser_domFullscreen_fullscreenMode.js]
> +[browser_removeme_fake_test.js]

You should remove this line.
Attachment #8621074 - Flags: review?
Attachment #8621074 - Attachment is obsolete: true
Attachment #8621079 - Flags: review?(jmaher)
Comment on attachment 8621079 [details] [diff] [review]
Removed the line from browser.ini

Review of attachment 8621079 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, this looks good and well defined.
Attachment #8621079 - Flags: review?(jmaher) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d95c168ab22b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.