Last Comment Bug 1022342 - Make use of Assert.jsm in xpcshell tests
: Make use of Assert.jsm in xpcshell tests
Status: RESOLVED FIXED
[good first bug][lang=js]
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: 4.0.0.1
Assigned To: Kushan Joshi [:kepta]
:
:
Mentors: Markus Adrario [:Taraman]
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-08 04:40 PDT by Philipp Kewisch [:Fallen]
Modified: 2015-02-22 16:51 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
bug1022342.patch (205.40 KB, patch)
2015-02-17 14:17 PST, Kushan Joshi [:kepta]
philipp: feedback+
Details | Diff | Splinter Review
bug1022342.patch (205.51 KB, patch)
2015-02-18 01:56 PST, Kushan Joshi [:kepta]
no flags Details | Diff | Splinter Review
bug1022342.patch (212.29 KB, patch)
2015-02-18 14:30 PST, Kushan Joshi [:kepta]
no flags Details | Diff | Splinter Review
bug1022342.patch (213.51 KB, patch)
2015-02-22 13:12 PST, Kushan Joshi [:kepta]
philipp: review+
Details | Diff | Splinter Review

Description User image Philipp Kewisch [:Fallen] 2014-06-08 04:40:00 PDT
There is now Assert.jsm, which is globally available in xpcshell tests. Here is what we can change:

  * do_check_eq(a, b) —>  equal(a, b)
  * do_check_neq(a, b) —> notEqual(a, b)
  * do_check_true(expr) —> ok(expr)
  * do_check_false(expr) —> ok(!expr)
  * do_check_null(expr) —> equal(expr, null)
  * do_check_matches(a, b) —> deepEqual(a, b) (undocumented XPCShell-test feature)

See https://groups.google.com/d/msg/mozilla.dev.platform/vdyLJBJTdYY/KudBBp8oaD4J for the announcement.
Comment 1 User image Philipp Kewisch [:Fallen] 2014-06-08 04:44:10 PDT
Note however there was some tl;dr; discussion on that thread. Some people mentioned that this should be changed on a more general basis in the test harness first. Please check first before starting work on this bug.
Comment 2 User image Matthew Schranz [:mjschranz] 2014-06-10 05:37:57 PDT
Interested in expanding my work across the many platforms of Mozilla so I hope no one minds I take this bug!

Would there be any recommended area's to tackle first?
Comment 3 User image Philipp Kewisch [:Fallen] 2014-06-11 13:08:30 PDT
Matthew, thanks for looking into this. I think a good start would be to read through the thread and figure out the general consensus on how and if it should be used in xpcshell tests. I'd appreciate a short summary if its not too much to ask :-)

Afterwards, I'd just start with any simple test in <http://mxr.mozilla.org/comm-central/source/calendar/test/unit/> We don't have terribly many unit tests, so its probably best to just go through all of them in that directory. For tests outside of that directory, i.e in toolkit, I'm sure other bugs have been filed. This one is specific to calendar.
Comment 4 User image Philipp Kewisch [:Fallen] 2015-02-17 10:57:30 PST
Welcome, a new contestant! Kushan will start working on this bug.
Comment 5 User image Kushan Joshi [:kepta] 2015-02-17 14:17:22 PST
Created attachment 8565671 [details] [diff] [review]
bug1022342.patch

Thanks Fallen for adding me.

I have replaced most of the code, please review the code.
( I would need a bit of help for do_check_throws, as simply replacing it with assert doesn't work)

-Thanks :D
Comment 6 User image Philipp Kewisch [:Fallen] 2015-02-17 15:36:42 PST
Comment on attachment 8565671 [details] [diff] [review]
bug1022342.patch

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

Could you let me know what troubles you are having with do_check_throws?

I've skimmed the code and have a few comments, I'd appreciate if you could fix them for the next iteration.

For future reference, if there are still open questions like the do_check_throws thing, please use the feedback flag instead of review. The review flag means "I think this is ready to be pushed, please take a final look", while feedback means something more like "This is already looking quite ok, but I would like some feedback on the general direction or have a question".

::: calendar/test/unit/head_consts.js
@@ +191,1 @@
>                      getProps(aRightItem,aPropArray[i]),

Make sure the indent for continuation lines is correct afterwards, it should be in this case:

equal(getProps(aLeftItem, aPropArray[i]),
      getProps(aRightItem,aPropArray[i]));

@@ +191,2 @@
>                      getProps(aRightItem,aPropArray[i]),
>                      Components.stack.caller);

I'm not quite sure, but I believe the third argument was removed?

::: calendar/test/unit/test_calmgr.js
@@ +157,3 @@
>  
>      // And be in the list of calendars
> +    ok(memory == calmgr.getCalendarById(memory.id));

Can be converted to use equal().
Comment 7 User image Kushan Joshi [:kepta] 2015-02-18 01:56:47 PST
Created attachment 8565888 [details] [diff] [review]
bug1022342.patch

Fallen, here is the patch with the changes.
Comment 8 User image Kushan Joshi [:kepta] 2015-02-18 04:50:54 PST
Is this the correct way to implement throws?
 
throws(function() {
                a[properties[i]] = old + 1;
      }, /Can not modify immutable data container/,"The object is immutable");
Comment 9 User image Philipp Kewisch [:Fallen] 2015-02-18 06:55:08 PST
Does it work?
Comment 10 User image Kushan Joshi [:kepta] 2015-02-18 11:43:31 PST
Yes it works.
Comment 11 User image Kushan Joshi [:kepta] 2015-02-18 14:30:10 PST
Created attachment 8566243 [details] [diff] [review]
bug1022342.patch

I have updated most of the code.
But I am stuck here-
  http://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_recur.js#520

  http://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_utils.js#124

If I try to find the error code value, the xpcshell gets stuck. Can you please help?
Comment 12 User image Philipp Kewisch [:Fallen] 2015-02-19 04:56:43 PST
Comment on attachment 8566243 [details] [diff] [review]
bug1022342.patch

A few things you can do:

* If the execution just stalls, then its waiting for a promise to complete. Sometimes you can just press Ctrl+C to at least show the error messages

* You can wrap it in a try/catch block and use do_print() to print the result.

* You can also pass a function as the "expected" value, then this function will be called with the actual result, i.e. 

throws(() => {
  // Something that throws NS_ERROR_FAILURE
}, err => err.result == Cr.NS_ERROR_FAILURE);
Comment 13 User image Kushan Joshi [:kepta] 2015-02-22 13:12:50 PST
Created attachment 8567699 [details] [diff] [review]
bug1022342.patch

Sorry for a late reply.
I have completed the patch from my side.
Please give a feedback.
Thanks :D
Comment 14 User image Philipp Kewisch [:Fallen] 2015-02-22 16:22:47 PST
Comment on attachment 8567699 [details] [diff] [review]
bug1022342.patch

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

Looks good, tests pass. r=philipp

Thanks for the patch!
Comment 15 User image Philipp Kewisch [:Fallen] 2015-02-22 16:51:18 PST
Pushed to comm-central changeset ea9366c07572

Note You need to log in before you can comment on or make changes to this bug.