458190 - Broken unit tests

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Daniel Boelzle [:dbo]]

The units are broken, and take quite long (longer than on 1.8 branch) to execute. Tested on comm-central Mac x86 debug build:

1. I see most of the js tests passing, but bus errors occur on every test, e.g.
*** exiting
*** PASS ***
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Volumes/data/Users/dbo/moz_cc/src/mozilla/xpcom/base/nsExceptionService.cpp, line 194

<<<<<<<
/Volumes/data/Users/dbo/moz_cc/src/mozilla/tools/test-harness/xpcshell-simple/test_all.sh: line 111: 19790 Bus error               NATIVE_TOPSRCDIR="$native_topsrcdir" TOPSRCDIR="$topsrcdir" $xpcshell -s $headfiles -f $t $tailfiles 2> $t.log 1>&2
TEST-UNEXPECTED-FAIL | ../../mozilla/_tests/xpcshell-simple/test_calendar/unit/test_recur.js | test failed, see log
../../mozilla/_tests/xpcshell-simple/test_calendar/unit/test_recur.js.log:
>>>>>>>

2. I see some strict warnings:
../../mozilla/_tests/xpcshell-simple/test_calendar/unit/test_recur.js:207: strict warning: variable item redeclares argument:
../../mozilla/_tests/xpcshell-simple/test_calendar/unit/test_recur.js:207: strict warning:     var item = makeEvent("DTSTART:20020402T114500Z\n" +

../../mozilla/_tests/xpcshell-simple/test_calendar/unit/test_recur.js:207: strict warning: ........^

3. I see test_recur.js failing:
*** TEST-UNEXPECTED-FAIL | ../../mozilla/_tests/xpcshell-simple/test_calendar/unit/test_recur.js | RRULE:FREQ=WEEKLY;COUNT=6;BYDAY=TU,WE
 == RRULE:FREQ=WEEKLY;COUNT=6;INTERVAL=1;BYDAY=TU,WE

I assume this is due to the libical update we've discussed (DTSTART not matching RRULE).

Comment 1

[Sebastian Schwieger]

Additionally to the reported error 1, I get a core dump of the test_all.sh script (with a linux-x64 build). I narrowed the cause down to the providers test.

Comment 2

[Martin Schröder [:mschroeder]]

Attachment: Patch (remove strict warning)

Comment 3

[Martin Schröder [:mschroeder]]

Attachment: [checked in] Patch (remove strict warning)

Comment 4

[Martin Schröder [:mschroeder]]

Comment on attachment 342073 [details] [diff] [review]
[checked in] Patch (remove strict warning)

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/e7012863d522>

Comment 5

[Stefan Sitter [:ssitter]]

When trying to run unittests for sunbird win32 debug build I get:

- A popup "xpcshell.exe - The instruction at '0x...' referenced memory at '0x...'. The memory could not be read. - Click on OK to terminate the program" for each test.

- The nsExceptionService warning mentioned above for each test

- test_alarm.js:

Checking repeatOffset...OK!
WARNING: Declared InterfaceInfo not found: file e:/dev/comm-central/src/mozilla/xpcom/reflect/xptinfo/src/xptiInterfaceInfo.cpp, line 434

- test_datetime.js and test_providers.js:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "Cc['@mozilla.org/xre/app-info;1'] is undefined" {file: "file:///e:/obj/sb/mozilla/dist/bin/components/nsExtensionManager.js" line: 2425}]' when calling method: [nsIFactory::createInstance]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///e:/obj/sb/mozilla/dist/bin/components/calItemModule.js -> file:///e:/obj/sb/mozilla/dist/bin/js/calTimezoneService.js :: calTimezoneService_ensureInitialized :: line 143"  data: yes]
************************************************************

- test_datetime.js:
line 67: strict warning: assignment to undeclared variable cd_allday

Comment 6

[Stefan Sitter [:ssitter]]

The xpcshell crash seems to be the same as in Bug 457024 [@nsXPCWrappedJS::Release] [@nsCOMPtr<calITimezone>::~nsCOMPtr<calITimezone>]

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: omit INTERVAL=1

libical 0.40 seems to omit INTERVAL=1

Comment 8

[Stefan Sitter [:ssitter]]

I can confirm that the crash and assertions mentioned before are now gone. All tests except test_recur.js pass.

Comment 9

[Daniel Boelzle [:dbo]]

test_recur.js will be fixed with the patch attached to this bug.

BTW: do we still need the homegrown calendar tests (http://mxr.mozilla.org/comm-central/source/calendar/test/homegrown/)? Opinions?

Comment 10

[Martin Schröder [:mschroeder]]

Comment on attachment 345474 [details] [diff] [review]
omit INTERVAL=1

I don't think removing 'INTERVAL=1' is the right thing to do. IMO you shouldn't correct an unit test if it's basically correct and valid iCalendar data. Maybe there is some lack of knowledge on my side, so please correct my if I'm wrong.

Comment 11

[Martin Schröder [:mschroeder]]

(In reply to comment #9)
> BTW: do we still need the homegrown calendar tests
> (http://mxr.mozilla.org/comm-central/source/calendar/test/homegrown/)?
> Opinions?

We don't need them anymore if we cover the tested functionality with XPCShell unit tests, otherwise we should convert the remaining homegrown calendar tests to XPCShell unit tests.

Comment 12

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> (From update of attachment 345474 [details] [diff] [review])
> I don't think removing 'INTERVAL=1' is the right thing to do. IMO you shouldn't
> correct an unit test if it's basically correct and valid iCalendar data. Maybe
> there is some lack of knowledge on my side, so please correct my if I'm wrong.

INTERVAL=1 is semantically equal to just omitting it which libical does. Aside from this I think the recurring unit tests are a bit weak, because they rely on a certain order of the RRULE params.

(In reply to comment #11)
> (In reply to comment #9)
> > BTW: do we still need the homegrown calendar tests
> > (http://mxr.mozilla.org/comm-central/source/calendar/test/homegrown/)?
> > Opinions?
> 
> We don't need them anymore if we cover the tested functionality with XPCShell
> unit tests, otherwise we should convert the remaining homegrown calendar tests
> to XPCShell unit tests.

Sure. My question is more like what tests are yet uncovered.

Comment 13

[Martin Schröder [:mschroeder]]

(In reply to comment #12)
> (In reply to comment #10)
> INTERVAL=1 is semantically equal to just omitting it which libical does. Aside
> from this I think the recurring unit tests are a bit weak, because they rely on
> a certain order of the RRULE params.

Agreed. Please file a follow-up bug for making the |test_interface()| function in |test_recur.js| more robust.

> (In reply to comment #11)
> > (In reply to comment #9)
> > We don't need them anymore if we cover the tested functionality with XPCShell
> > unit tests, otherwise we should convert the remaining homegrown calendar tests
> > to XPCShell unit tests.
> 
> Sure. My question is more like what tests are yet uncovered.

Okay, we just need a follow-up bug, which you can assign to me.

Comment 14

[Martin Schröder [:mschroeder]]

Comment on attachment 345474 [details] [diff] [review]
omit INTERVAL=1

r=mschroeder

Comment 15

[Daniel Boelzle [:dbo]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/f2d8cfd2bf2d>

-> FIXED

bug 462823 and bug 462824 filed

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".