Make use of Assert.jsm in xpcshell tests

RESOLVED FIXED in 4.0.0.1

Status

Calendar
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Fallen, Assigned: kepta, Mentored)

Tracking

unspecified
4.0.0.1
x86
Mac OS X

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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.
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?
Assignee: nobody → schranz.m
Status: NEW → ASSIGNED
(Reporter)

Comment 3

3 years ago
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.
Mentor: Mozilla@Adrario.de
Whiteboard: [good first bug][mentor=Taraman][lang=js] → [good first bug][lang=js]
Assignee: schranz.m → nobody
Status: ASSIGNED → NEW
(Reporter)

Comment 4

3 years ago
Welcome, a new contestant! Kushan will start working on this bug.
Assignee: nobody → 0o3ko0
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
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
Attachment #8565671 - Flags: review?(philipp)
(Reporter)

Comment 6

3 years ago
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().
Attachment #8565671 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8565888 [details] [diff] [review]
bug1022342.patch

Fallen, here is the patch with the changes.
Attachment #8565671 - Attachment is obsolete: true
Attachment #8565888 - Flags: feedback?(philipp)
(Assignee)

Comment 8

3 years ago
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");
(Reporter)

Comment 9

3 years ago
Does it work?
(Assignee)

Comment 10

3 years ago
Yes it works.
(Assignee)

Comment 11

3 years ago
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?
Attachment #8565888 - Attachment is obsolete: true
Attachment #8565888 - Flags: feedback?(philipp)
Attachment #8566243 - Flags: feedback?(philipp)
(Reporter)

Comment 12

3 years ago
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);
Attachment #8566243 - Flags: feedback?(philipp)
(Assignee)

Comment 13

3 years ago
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
Attachment #8566243 - Attachment is obsolete: true
Attachment #8567699 - Flags: feedback?(philipp)
(Reporter)

Comment 14

3 years ago
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!
Attachment #8567699 - Flags: feedback?(philipp) → review+
(Reporter)

Comment 15

3 years ago
Pushed to comm-central changeset ea9366c07572
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.0
You need to log in before you can comment on or make changes to this bug.