Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#3729 closed defect (fixed)

dojo.data tests fail on Rhino

Reported by: Adam Peller Owned by: skinner
Priority: high Milestone: 0.9
Component: General Version: 0.9
Keywords: Cc: Jared Jurkiewicz
Blocked By: Blocking:

Description

In a big way... asserts fail, followed by a StackOverflow? Error!

I'd suggest that rather than an if/else, we test the same things regardless of platform. So, break out the first test, and have it run only on browsers. Then, have the else part run as a second test everywhere.

Attachments (1)

dojo.data_ItemStoreTemplates_20070711.patch (29.4 KB) - added by Jared Jurkiewicz 12 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 12 years ago by Jared Jurkiewicz

This seems to only fail with Rhino. for some reason Rhino hates the way Brian set up the associative map which returns the constructor data for the store. Basically this:

----------------------------------------------------- testFile data-sets tests.data.readOnlyItemFileTestTemplates.testFile = {};

if(dojo.isBrowser){

tests.data.readOnlyItemFileTestTemplates.testFilecountries? = {url: dojo.moduleUrl("tests", "data/countries.json").toString() };

}else{

tests.data.readOnlyItemFileTestTemplates.testFilecountries? = {data: {

identifier:"abbr", label:"name", items:[

{abbr:"ec", name:"Ecuador", capital:"Quito"}, {abbr:'eg', name:'Egypt', capital:'Cairo'}, {abbr:'sv', name:'El Salvador', capital:'San Salvador'}, {abbr:'gq', name:'Equatorial Guinea', capital:'Malabo'}, {abbr:'er', name:'Eritrea', capital:'Asmara'}, {abbr:'ee', name:'Estonia', capital:'Tallinn'}, {abbr:'et', name:'Ethiopia', capital:'Addis Ababa'}

]

} };

}

if(dojo.isBrowser){

tests.data.readOnlyItemFileTestTemplates.testFilecountries_withNull? = {url: dojo.moduleUrl("tests", "data/countries_withNull.json").toString() };

}else{

tests.data.readOnlyItemFileTestTemplates.testFilecountries_withNull? = {data: {

identifier:"abbr", items:[

{abbr:"ec", name:null, capital:"Quito"}, {abbr:'eg', name:null, capital:'Cairo'}, {abbr:'sv', name:'El Salvador', capital:'San Salvador'}, {abbr:'gq', name:'Equatorial Guinea', capital:'Malabo'}, {abbr:'er', name:'Eritrea', capital:'Asmara'}, {abbr:'ee', name:null, capital:'Tallinn'}, {abbr:'et', name:'Ethiopia', capital:'Addis Ababa'}

]

} };

}

If you access the countires attribute of the map ... you get back what 'looks' like an empty object. But if you then do a dojo.toJson() on it, it explodes rhino with a huge stack dump.

Simple fix seems to be to ditch the map and just do a function that takes a name and retirns the appropriate dataset constructor. That fixes it and works in both browser and rhino. Will be checking in a fix for that .

Changed 12 years ago by Jared Jurkiewicz

comment:2 Changed 12 years ago by Jared Jurkiewicz

I think somehow in the tests the static map data was getting shared and corrupted when being run in rhino in the tests. By moving it to a function, it makes sure the data isn't static between the stores and gets it working cleanly.

This does warrant further investigation, but at least it gets the testcases working in Rhino without a horrific stack dump.

comment:3 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [9595]) Update to testcases to get around a problem with Rhino running the UT. Further investigation should still be done.... fixes #3729

comment:4 Changed 12 years ago by skinner

I did some more debugging, and here's what I think:

This is bug in ItemFileReadStore, not a problem with the unit tests, and not a problem with Rhino unit tests. We happened to encounter the bug when testing in Rhino, but only because we were actually testing a different path through the ItemFileReadStore. When we ran the tests in Rhino, we always called the ItemFileReadStore constructor using a 'data:' keyword rather than a 'url:' keyword. The same unit tests will also fail when run in the browser, if you just change the tests to use the 'data:' loading feature instead of the 'url:' loading feature.

Here's the root bug: if you pass some 'data:' objects to the !ItemFileReadStore constructor, !ItemFileReadStore does not make a copy of those objects when it turns them into items. Instead, !ItemFileReadStore just adds its own private properties to the objects, and it continues to use the objects as if it is the sole owner of them. If you pass the same set of objects as 'data:' to the constructor of a second ItemFileReadStore, then that second datastore will also try to act as the sole owner of those objects, which then causes all sorts of problems.

This bug might cause problems in other situations as well. Having two ItemFileReadStore instance share the same initialization 'data:' objects is just one scenario where the bug appears. The bug might also cause problems in any situation where you first create an !ItemFileReadStore by passing 'data:' objects, and then continue to use those objects in any other part of your program, anywhere outside the ItemFileReadStore.

One obvious solution is to have the ItemFileReadStore constructor always make a copy of all of the objects that were passed in using the 'data:' keyword. However, that solution entails a serious performance hit. If the datastore is being passed a large amount of initialization data, then we have to copy a lot of objects all at once just to create the datastore -- and that copy step may be completely unnecessary if the original objects were never going to be used for anything else anyway.

Another solution is to get rid of the 'data:' keyword, and replace it with two keywords that have slightly different semantics. With one keyword, the data would always be copied. With the other keyword, the data would always be consumed, and would not be available for further use. But that's probably a bad idea, just because it's too complicated.

A third solution is simply to make an effort to carefully document the fact that ItemFileReadStore will always consume any data objects that you feed to its constructor, and warn you that you must never try to use those objects for anything else afterwards.

Note: See TracTickets for help on using tickets.