Closed Bug 518436 Opened 10 years ago Closed 2 years ago

Move do_check_throws into testing/xpcshell/head.js

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jorendorff, Unassigned)

References

Details

(Whiteboard: [lang=js][apparently awful first bug])

Attachments

(3 files)

Attached patch v1Splinter Review
Recently added in a js-ctypes test. Ted claims it's generally useful.

Not tested yet. I'll mark the patch r?ted after a local build and make check.
I have a few cases here that could be changed to use it, definitely:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/unit/test_crashreporter.js#70

I'm sure there are more throughout the tree, but you don't have to change them all just to land this, of course.
Attachment #402438 - Flags: review?(ted.mielczarek)
Comment on attachment 402438 [details] [diff] [review]
v1

Oh wait, the do_check_throws I wrote doesn't check exc.result codes.

I can fix it later today, I think. A couple things are ahead in the queue.
Attachment #402438 - Flags: review?(ted.mielczarek)
Ok. Would also be cool to have some basic sanity tests of this in testing/xpcshell/example/unit:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/example/unit/
Duplicate of this bug: 650402
Locations and soon-to-be-dupes:

Bug 913199

content/base/test/unit/test_thirdpartyutil.js
dom/mobilemessage/tests/header_helpers.js
dom/mobilemessage/tests/test_smsservice_createsmsmessage.js
dom/system/gonk/tests/header_helpers.js
extensions/cookie/test/unit/head_cookies.js
modules/libpref/test/unit/head_libPrefs.js
services/common/tests/unit/head_helpers.js
services/crypto/component/tests/unit/test_jpake.js
services/sync/tests/unit/test_service_cluster.js
toolkit/components/contentprefs/tests/unit_cps2/head.js
toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
toolkit/modules/tests/xpcshell/test_FileUtils.js
OS: Mac OS X → All
Hardware: x86 → All
Marking this as a mentor bug, so there's a chance it gets done.
Whiteboard: [mentor=rnewman][lang=javascript][good first bug]
Hi i'd like to work on it as my first bug? any idea how to start ?
(In reply to AdityaSIngh [:MacroMayhem] from comment #7)
> Hi i'd like to work on it as my first bug? any idea how to start ?

First, get started with building desktop Firefox:

https://developer.mozilla.org/docs/Simple_Firefox_build

and reach the point where you can run 

  ./mach xpcshell-test 

and have the tests pass. Let us know when you get to that point!
(In reply to Richard Newman [:rnewman] from comment #8)
> (In reply to AdityaSIngh [:MacroMayhem] from comment #7)
> > Hi i'd like to work on it as my first bug? any idea how to start ?
> 
> First, get started with building desktop Firefox:
> 
> https://developer.mozilla.org/docs/Simple_Firefox_build
> 
> and reach the point where you can run 
> 
>   ./mach xpcshell-test 
> 
> and have the tests pass. Let us know when you get to that point!

I ran the command but i am unsure if it was successful. the output contained success, failures and TODOs too.
 output for ./mach xpcshell-test  https://pastebin.mozilla.org/4445187
OK, that's good enough to get started.

To resolve this bug:

* Find each location of a definition of do_check_throws. I already did this in Comment 5.
* Identify which definitions are identical.
* Replace the usages of those definitions with calls to a single definition in testing/xpcshell/head.js.
* Verify that xpcshell tests continue to pass.
* Any definitions that aren't identical, reconcile their capabilities.
I replaced the do_check_throws calls in content/base/test/unit/test_thirdpartyutil.js to do_check_throws_nsIException in testing/xpcshell/head.js. https://pastebin.mozilla.org/4450782

I did a ./mach xpcshell-test to check if it didn't mess things up. Following were the results. https://pastebin.mozilla.org/4450785 

let me know if i am going in the right direction.
(In reply to AdityaSingh [:MacroMayhem] from comment #12)
> I replaced the do_check_throws calls in
> content/base/test/unit/test_thirdpartyutil.js to
> do_check_throws_nsIException in testing/xpcshell/head.js.

That's probably not right. d_c_t_nsIException checks that an nsIException is raised, which is more strict than do_check_throws.

You should start by trying to reconcile all of the different implementations of do_check_throws that I listed in Comment 5.
I implemented the do_check_throws in testing/xpcshell/head.js the implementation has been removed from all the files except 
> services/crypto/component/tests/unit/test_jpake.js
> services/sync/tests/unit/test_service_cluster.js
> toolkit/components/contentprefs/tests/unit_cps2/head.js
i ran xpcshell-test for other files and they successfully finished.
Implementation is different in these 3 files. I have attached new do_check_throws() and the one which couldn't be removed. https://pastebin.mozilla.org/4456862..
also i noticed do_check_throws_message() in services/common/tests/unit/head_helpers.js which serves a similar functionality.
Modifications can be made to the remaining 3 .js which still have "do_check_throws" method in them.
Attachment #8385081 - Flags: feedback?(rnewman)
Comment on attachment 8385081 [details] [diff] [review]
514836.patch :moves "do_check_throws()" to a common place for all but 3 files.

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

There are a number of different implementations here. Some check constructors, some check result codes, some check error message. You need different do_check methods for each one. I would be really surprised if this patch didn't cause a number of tests to fail.

::: dom/mobilemessage/tests/header_helpers.js
@@ -27,5 @@
> -
> -  try {
> -    func();
> -  } catch (ex) {
> -    if (ex.name == result) {

This is not the same as the one you replaced it with. This one checks ex.name, yours checks ex.result.

::: dom/system/gonk/tests/header_helpers.js
@@ -172,5 @@
>  
> -  try {
> -    func();
> -  } catch (exc) {
> -    if (exc.message === message) {

Note that this is not the same as the one you replaced it with.

This one checks ex.message. Yours checks ex.result.

::: testing/xpcshell/head.js
@@ +901,5 @@
>    }
>    return indent + a;
>  }
>  
> +function do_check_throws(func, result, stack)

This should really be "do_check_throws_result", which will require you to fix up tests in the locations you're editing.

@@ +905,5 @@
> +function do_check_throws(func, result, stack)
> +{
> +	if(!stack)
> +	{
> +		try

This needs to be adjusted to match our coding style:

function do_check_throws(func, result, stack) {
  if (!stack) {
    try {
      stack = Components.stack.caller;
    } catch (e) {
    }
  }

  try {
    func();
  } catch (ex) {
    if (ex.result == result) {
      return;
    }
    do_throw("Expected " + result + ", caught " + ex, stack);
  }

  do_throw("Expected exception " + result + ", none thrown.", stack);
}

@@ +914,5 @@
> +	try
> +	{
> +		func();
> +	}catch(exc){
> +		if(exc.result == result)

Just 

  do_check_eq(ex.result, result);

::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
@@ -25,5 @@
> -
> -  try {
> -    f();
> -  } catch (exc) {
> -    if (exc.constructor.name === type.name) {

This is not the same as the one you replaced it with. This one checks constructors (via name), yours checks result.
Attachment #8385081 - Flags: feedback?(rnewman) → feedback-
mozilla-central/services/common/tests/unit: do_check_throws makes use of do_check_eq but it passes 3 variables which is against the definition of 2 in the documentation https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests. Any suggestions?
Attached patch 518436v2.patchSplinter Review
I need a little help in moving the function from 
services/common/tests/unit/head_helpers.js
services/crypto/component/tests/unit/test_jpake.js
services/sync/tests/unit/test_service_cluster.js
toolkit/components/contentprefs/tests/unit_cps2/head.js
Attachment #8388086 - Flags: feedback?(rnewman)
Mentor: rnewman
Whiteboard: [mentor=rnewman][lang=javascript][good first bug] → [lang=javascript][good first bug]
rnewman, this patch has been idle for 4 months, any issues via irc?
Flags: needinfo?(rnewman)
I think I've decided that this is not a good bug to mentor. Ten minutes for someone who knows what they're doing, versus dozens of hours of mentoring. Frustration at both ends.
Mentor: rnewman
Flags: needinfo?(rnewman)
Whiteboard: [lang=javascript][good first bug] → [lang=javascript][apparently awful first bug]
Attachment #8388086 - Flags: feedback?(rnewman)
Whiteboard: [lang=javascript][apparently awful first bug] → [lang=js][apparently awful first bug]
Mass closing bugs with no activity in 2+ years. If this bug is important to you, please re-open.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.