Closed Bug 458190 Opened 11 years ago Closed 11 years ago

Broken unit tests

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

Attachments

(2 files, 1 obsolete file)

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).
Flags: blocking-calendar1.0+
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.
Attached patch Patch (remove strict warning) (obsolete) — Splinter Review
Attachment #342068 - Flags: review?(daniel.boelzle)
Attachment #342068 - Attachment is obsolete: true
Attachment #342073 - Flags: review?(daniel.boelzle)
Attachment #342068 - Flags: review?(daniel.boelzle)
Attachment #342073 - Flags: review?(daniel.boelzle) → review+
Attachment #342073 - Attachment description: Patch (remove strict warning) → [checked in] Patch (remove strict warning)
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>
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
The xpcshell crash seems to be the same as in Bug 457024 [@nsXPCWrappedJS::Release] [@nsCOMPtr<calITimezone>::~nsCOMPtr<calITimezone>]
Attached patch omit INTERVAL=1Splinter Review
libical 0.40 seems to omit INTERVAL=1
Assignee: nobody → daniel.boelzle
Attachment #345474 - Flags: review?(mschroeder)
I can confirm that the crash and assertions mentioned before are now gone. All tests except test_recur.js pass.
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 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.
(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.
(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.
(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.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → 1.0
Comment on attachment 345474 [details] [diff] [review]
omit INTERVAL=1

r=mschroeder
Attachment #345474 - Flags: review?(mschroeder) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/f2d8cfd2bf2d>

-> FIXED

bug 462823 and bug 462824 filed
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.