1022342 - Make use of Assert.jsm in xpcshell tests

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Matthew Schranz [:mjschranz]]

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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Welcome, a new contestant! Kushan will start working on this bug.

Comment 5

[Kushan Joshi [:kepta]]

Attachment: 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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Kushan Joshi [:kepta]]

Attachment: bug1022342.patch

Fallen, here is the patch with the changes.

Comment 8

[Kushan Joshi [:kepta]]

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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Does it work?

Comment 10

[Kushan Joshi [:kepta]]

Yes it works.

Comment 11

[Kushan Joshi [:kepta]]

Attachment: 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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Kushan Joshi [:kepta]]

Attachment: bug1022342.patch

Sorry for a late reply.
I have completed the patch from my side.
Please give a feedback.
Thanks :D

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset ea9366c07572