Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#3679 closed defect (fixed)

Unit tests fail in various timezones

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

Description

The current dojo.data tests assume Pacific Time and fail elsewhere. Either the tests have to be adjusted to account for this, or perhaps dojo.date.stamp could be used to serialization instead? We should discuss whether this is a matter of 'best practices' or something that belongs in the API.

Attachments (1)

dojo.data_ItemStoreFixes_20070711.patch (11.5 KB) - added by Jared Jurkiewicz 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by skinner

oops -- sorry about the failing unit tests.

I'm no expert on date and timezone representations, so I'm out of my depth here, but using dojo.date.stamp sounds like a good idea to me.

Our current dojo.data tests are using numbers like "738226800000" to represent dates in terms of milliseconds since 1970. That format sucks because it's not human-readable, and I guess it has timezone problems too. Another obvious alternative is to use a string format built into the JavaScript core Date object -- a string format that would be accepted by Date.parse(), but unfortunately the ECMAScript standard actually doesn't specify that string format.

I do think we should update dojo.data.ItemFileReadStore so that it has better support for date literals. But I'm not sure that the Read API itself needs to know about date literals, given that we will have datastores like CsvStore that only ever return string literals, never number literals or date literals.

comment:2 Changed 12 years ago by Adam Peller

Brian, take a look at dojo.date.stamp. It's pretty simple, and I think we should use it consistently throughout Dojo for serialization.

Date.parse() is broken. Stay away from it, and also try to avoid setting dates like new Date("May 23, 2005") as that's not spec'd either.

comment:3 Changed 12 years ago by skinner

Owner: changed from skinner to Jared Jurkiewicz

comment:4 Changed 12 years ago by Jared Jurkiewicz

Working on a patch for this. My solution to it is to actually change the typeMap argument of the store a bit to allow for a structure like:

{

type: {

deserializer: function(value){}; serializer: function() {};

}

}

Instead of just a constructor. This allows users to define how they want to deserialize and serialize types easily. In many cases the deserializer may just be something like:

return new Color(value);

But then it also allows us to do for date: return dojo.date.stamp.fromIsoString(value);

And for a serializer: return dojo.date.stamp.toIsoString(Date,{zulu:true});

To force serialization in ISO date format for UTC as the default for Date.

This should also make it somewhat easy to do serialization of the type ... I hope. Still need to look at that more.

comment:5 Changed 12 years ago by Adam Peller

sounds great. It's the many degrees of freedom we often try to stay away from in 0.9 core, but it's probably justified here.

It occurred to me after the meeting that unless you use a date stamp with just 'date' vs 'datetime' that you're still going to have pretty much the same "what day is it in my timezone" problem as you have with the int. So it really depends on the use case. If someone means to simply specify a day and not a point in time, I think it's going to be ambiguous unless they use a serializer with the 'date' option. FYI.

comment:6 Changed 12 years ago by Jared Jurkiewicz

Adam,

The default serializer for Date objects is going to be the full datetime in UTC (zulu) mode to avoid issues with Timezoning. Effectively:

if(!this._datatypeMapDate?.serialize){

this._datatypeMapDate?.serialize = function(obj){

return dojo.date.stamp.toISOString(obj, {zulu:true});

}

}

And I doubt people would often be over-riding the serializer. If they do, though, it's their problem to deal with timezones/ints. :)

Changed 12 years ago by Jared Jurkiewicz

comment:7 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [9587]) Checking in fix to the date serializer to use dojo.date.stamp to use ISO formats for storage and default serialize/deserialize. fixes #3679

comment:8 Changed 12 years ago by Jared Jurkiewicz

Tested on:

IE 6.0 IE 7.0 Firefox 2.0.0.4 Opera 9.2 Safari Beta 3 SeaMonkey? 1.1.2

comment:9 Changed 12 years ago by skinner

That fix works nicely for Dates, but the downside is that it complicates the typeMap registration code for custom data classes. For example, here's the old code to register dojo.Color as the handler for Color literals:

var store = new dojo.data.ItemFileWriteStore({
    data: dataSet,
    typeMap: {'Color': dojo.Color}
});

And here's the code that's now required after the fix:

var store = new dojo.data.ItemFileReadStore({
    data: dataSet,
    typeMap: {'Color': {
        type: dojo.Color,
        deserialize: function(value){
            return new dojo.Color(value);
        }
        serialize: function(obj){
            return obj.toString();
        }
    }}
});

As an alternative approach, what about just handling Dates as a special case -- instead of adding a default typeMap entry for 'Date', we just hard-code the logic for serializing and deserializing Dates. And then we could go back to using simple typeMap entries that look like "{'Color':dojo.Color}", rather than requiring typeMap entries to specify serialize: and deserialize: functions.

We could also, if we chose to, make it impossible to override the built-in logic for serializing and deserializing Dates, which I think might be a good thing.

comment:10 Changed 12 years ago by Adam Peller

can't you stick with the current approach, just default to the object's constructor and the toString method for deserialize/serialize, respectively?

Note: See TracTickets for help on using tickets.