Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calendar / Datepicker: improve tests and add missing events #1755

Merged
merged 17 commits into from Nov 9, 2016

Conversation

@fnagel
Copy link
Member

fnagel commented Sep 30, 2016

  • Fix datepicker min / max attribute parsing
  • Add change event incl. basic tests
  • Add unit tests for calendar select event
  • Add unit tests for calendar value option
  • Add unit tests for datepicker min / max option
  • Introduce dateEqual assertion as proposed in wiki
  • Make use of setup / teardown methods and remove helper file as proposed in wiki
@fnagel
Copy link
Member Author

fnagel commented Sep 30, 2016

@fnagel
Copy link
Member Author

fnagel commented Sep 30, 2016

Unit tests work fine in browser and cli when running locally. Any ideas?

Copy link
Member

jzaefferer left a comment

It would be a bit easier to get these landed with a smaller scope. Mixing refactoring unit tests and implementation changes isn't necessary, as far as I can tell.

As for the failing tests, that looks like a timezone issue. I can reproduce it locally by running tests in a different timezone, like this:

TZ=:/usr/share/zoneinfo/America/Sao_Paulo npm test

Specifically for calendar/datepicker, we should start doing this by default.

module( "calendar: core", {
setup: function() {
element = $( "#calendar" ).calendar();
widget = element.calendar( "widget" );

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

You could assign this.widget = here, then reference this.widget inside the test, to avoid the top-level vars.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 11, 2016

Author Member

Done.

}, 50 );
}

step1();

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

If this is called immediately, the function seems unnecessary. Could inline the setTimeout call here.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 3, 2016

Member

We do this a lot because it makes it easier to read the order of the test from top to bottom.

_handleKeydown: function( event ) {
var pageAltKey = ( event.altKey || event.ctrlKey && event.shiftKey );

switch ( event.keyCode ) {
case $.ui.keyCode.ENTER:
this.activeDescendant.mousedown();
case $.ui.keyCode.ENTER:

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

The indent shouldn't change.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 5, 2016

Author Member

Done.

_select: function( event ) {
var oldValue = this.options.value ? this.options.value.getTime() : "";

this._setOption( "value", new Date( $( event.currentTarget ).data( "timestamp" ) ) );

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

Could this access this.activeDescendant instead? Would make the ENTER key event handler simpler, since it wouldn't need to pass the event along.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 3, 2016

Author Member

You're suggesting to replace new Date( $( event.currentTarget ).data( "timestamp" ) ) with this.activeDescendant right?

Don't think so as this.activeDescendant would be the current day, not the selected one. this.activeDescendant will be updated in the next line, in _updateDayElement.

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

Okay, makes sense.


if ( $.type( this.options.max ) === "string" ) {
this.options.max = Globalize.parseDate( this.options.max, { raw: "yyyy-MM-dd" } );
this.options.max = this._parse( this.options.max );

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

According to http://wiki.jqueryui.com/w/page/12137778/Datepicker both min and max should be date, not string. Is that document out of date?

If so, wasn't it on purpose that we used a fixed iso format for min/max as string, instead of matching the dateFormat option? With this change, the developer would need to update min/max whenever the (user facing) dateFormat option changes.

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

With the parsing happening in _getCreateOptions, I'm even more confused, since that uses a different format.

Since I doubt these changes are happening by accident, let's update the spec.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 3, 2016

Author Member

The current implementation is:

  1. min / max option passed as date
  2. min / max option passed as string: format value according to dateFormat option
  3. min / max attributes on input element: format value as yyyy-MM-dd (according to HTML specs)

I'm not able to find the reference in the GH comments, but we wanted it this way and I guess we need to update the wiki specs.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 3, 2016

Member

I recall the discussions about this, but I don't recall where the discussions occurred. The main thing we want to support is Date instances. I believe what we wanted to do for strings was parse the string into a date and store the date, not the string. For attributes, in order to support valid HTML5, we must follow the spec, and not support custom formatting; we'd still store the options as Date instances.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 3, 2016

Author Member

I believe what we wanted to do for strings was parse the string into a date and store the date, not the string. For attributes, in order to support valid HTML5, we must follow the spec, and not support custom formatting; we'd still store the options as Date instances.

That's what we have now. We only store date objects and attributes formatting is hard coded.

Changing the wiki specs then, right?

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

The above makes sense. Let's update the spec with that.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 3, 2016

Author Member

Done.

@@ -172,6 +176,9 @@ var widget = $.widget( "ui.datepicker", {
},
blur: function() {
this.suppressExpandOnFocus = false;
},
change: function( event ) {

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 3, 2016

Member

Is the native change event reliable enough? Usually the answer to that is "no", may need to do a manual dirty-check in blur and trigger change from there.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 3, 2016

Author Member

Not sure. Had massive issues creating reliable tests for this use case.

Checking the blur events is komplex due to the timeout handling. I wasn't able to prevent additional events from firing.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

The native change event should be fine for user input. It's only programmatic value changes that would cause the event to not be fired.

@fnagel
Copy link
Member Author

fnagel commented Oct 3, 2016

It would be a bit easier to get these landed with a smaller scope. Mixing refactoring unit tests and implementation changes isn't necessary, as far as I can tell.

Totally agree. I will make sure next PRs are more specific.

As for the failing tests, that looks like a timezone issue. I can reproduce it locally by running tests in a different timezone [...]

Mhh, as always, it's not that easy when using windows. Not sure if it's even possible without changing system time globally... :-/

More important would be how to deal with this for the travis tests? Do we want to test multiple zones? Or just setting a specific one for the tests?

@jzaefferer
Copy link
Member

jzaefferer commented Oct 3, 2016

Mhh, as always, it's not that easy when using windows. Not sure if it's even possible without changing system time globally... :-/

Does Git Bash help? https://git-for-windows.github.io/

More important would be how to deal with this for the travis tests? Do we want to test multiple zones? Or just setting a specific one for the tests?

On another project where I ran into a similar problem, we decided to run the test suite twice, once on the CI machine's local timezone, once on a specific, different one. That way we make sure that the tests pass in different timezones. Not necessarily in all of them, but testing 2 is much better than just the default.

Its also possible that these issues only affect the testsuite, in which case it might be much more pragmatic to just set a fixed timezone.

} );

test( "change", function() {
expect( 6 );

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

This is using globals. Should use assert.expect() (assert gets passed as a parameter to the test() callback).

Looks like the datepicker branch is missing some updates to not allow the use of the QUnit globals. I guess we'll address that separately.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 4, 2016

Author Member

Ok, as I'm already doing this for the custom dateEqual assert this makes sense to me.

Looks like the datepicker branch is missing some updates to not allow the use of the QUnit globals.

A merge with master is needed anyway when this PR is merged.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 11, 2016

Author Member

Done.

equal(
event.originalEvent.type,
eventType,
"select originalEvent on calendar button " + eventType

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

s/select/change/

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 5, 2016

Author Member

Done.

message = "on calendar button " + eventType;
element.find( "table button:eq(1)" ).simulate( eventType );
step2();
}, 50 );

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

Unwrap the timeouts inside the steps. Use the timeout for invoking the next step:

function step1() {
    eventType = "mousedown";
    message = "on calendar button " + eventType;
    element.find( "table button:eq(1)" ).simulate( eventType );
    setTimeout( step2, 50 );
}

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 11, 2016

Author Member

Done.


var date = new Date( 2015, 8 - 1, 1 );

// Number of month option does not work after init

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

This needs to work. Can you just make this a TODO for now?

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 5, 2016

Author Member

Done.


element.calendar( "option", "value", date );
assert.dateEqual( element.calendar( "option", "value" ), date, "Value set" );
equal( widget.find( "table button.ui-state-active" ).data( "timestamp" ), 1463954400000, "Active button timestamp" );

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

We need to prefix our data keys, so this should be ui-calendar-timestamp.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 11, 2016

Author Member

Done.

equal( widget.find( "table button.ui-state-active" ).data( "timestamp" ), 1463954400000, "Active button timestamp" );

element.calendar( "option", "value", "invalid" );
assert.dateEqual( element.calendar( "option", "value" ), date, "Value after invalid parameter" );

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

Perhaps we've discussed this before, but I don't remember talking through this. Is there a reason we leave the previous value instead of reverting to null? Native date inputs will clear their value if you try to set an invalid value.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 4, 2016

Author Member

No, no reason afaik and I don't remember talking trough this. I'll see if this can be done easily within the _setOption method.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 12, 2016

Author Member

@scottgonzalez What about dates invalid due to min / max option? Upcoming change will include resetting value to null when it's not within min / max limit.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 12, 2016

Member

Those get reverted to null as well.

@@ -13,8 +13,7 @@
<div id="qunit"></div>
<div id="qunit-fixture">

<input type="text" id="datepicker">
<input type="text" id="datepicker2">
<input type="text" id="datepicker" />

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

Removing the trailing slash. We don't use XHTML.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 5, 2016

Author Member

Done.

@@ -79,30 +81,33 @@ var widget = $.widget( "ui.datepicker", {
_getCreateOptions: function() {
var max = this.element.attr( "max" ),
min = this.element.attr( "min" ),
parser = new Globalize( "en" ).dateParser( { raw: "yyyy-MM-dd" } ),

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

Not directly related to this PR, but does Globalize always contain the en data or do we need to tell users to always include this to handle the attribute parsing?

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 4, 2016

Author Member

Not sure, but I guess it needs to be included.

@rxaviers Can you help us out here?

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 12, 2016

Member

For this raw pattern, the locale might not be required, since there's nothing locale-specific, I think. In general, Globalize doesn't bundle any locales.

This comment has been minimized.

Copy link
@rxaviers

rxaviers Oct 13, 2016

Member

Actually, there's one detail.

since there's nothing locale-specific

For the above pattern, some stuff are locale independent, but some are locale-specific.

Locale independent:

  • Your pattern will result in numeric output only and given you're passing a raw format, the order of the fields are independent of any locale preferences.

Locale dependent:

  • The only difference you would get is numbering system, for example, by using Arabic you would can expect something like '٢٠١٦-١٠-١٣'.

This comment has been minimized.

Copy link
@rxaviers

rxaviers Oct 13, 2016

Member

Please, refresh my mind here? Is this parser being used to parse developer input? If so, I believe it's expected to be in English (or English POSIX, or root) form.

By the way, this is a very simple parsing, so it's also worth considering not using Globalize and instead split the string at '-' and constructing the Date using the array pieces.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 19, 2016

Member

+1 for manual parsing without Globalize

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 24, 2016

Author Member

When using something like

parser = function( value ) {
    var exploded = value.split( "-" );

    return new Date( exploded[ 0 ], exploded[ 1 ], exploded[ 2 ] );
},

I'm running into timezone issues:
image

Any ideas?

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 25, 2016

Member

I think you need to set the correct offset for the month, since it is defined as "Integer value representing the month, beginning with 0 for January to 11 for December."

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 25, 2016

Member

Like tests are littered with x - 1 for the month everywhere.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 25, 2016

Author Member

No idea why, but I was focused on the missing hour not the month. Stupid me.

Pushed a new commit with this.

@@ -172,6 +176,9 @@ var widget = $.widget( "ui.datepicker", {
},
blur: function() {
this.suppressExpandOnFocus = false;
},
change: function( event ) {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

The native change event should be fine for user input. It's only programmatic value changes that would cause the event to not be fired.

},

_setOption: function( key, value ) {
if ( key === "max" || key === "min" ) {
if ( $.type( value ) === "string" ) {

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

Just use typeof.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 4, 2016

Author Member

Sure. Just for my curiosity: Why not using the internals?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Oct 4, 2016

Member

jQuery's type detection is mostly unneeded in modern browsers. For strings, it was never needed. $.type() was just an abstraction away from the $.is*() methods which were limited to types that mattered.

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 5, 2016

Author Member

Thanks for the info.

Done.

@fnagel
Copy link
Member Author

fnagel commented Oct 4, 2016

Does Git Bash help? https://git-for-windows.github.io/

@jzaefferer Already using git bash, but tried power shell as well :-/

@fnagel
Copy link
Member Author

fnagel commented Oct 5, 2016

Added timezone / travis functionality as a todo task in wiki.

@jzaefferer
Copy link
Member

jzaefferer commented Oct 12, 2016

Can you get the test to fail locally by changing your system timezone? Have you tried running tests inside a Docker container? If not, I can help set that up.

@fnagel
Copy link
Member Author

fnagel commented Oct 12, 2016

Oh man, changing the system timezone via the UI works (but does not using similar console command). Seems this messes with some of local tools but for a quick test this seems to work.

@fnagel
Copy link
Member Author

fnagel commented Oct 12, 2016

Have you tried running tests inside a Docker container? If not, I can help set that up.

I'm using VirtualBox / Vagrant VMs for other projects but always tried to get around this for jQuery UI.
And hey, perhaps it's a good idea to have at least a one developer building the project on Windows, right?

@fnagel fnagel force-pushed the fnagel:datepicker-working branch from aaac0d5 to 0891b50 Oct 12, 2016
@fnagel
Copy link
Member Author

fnagel commented Oct 12, 2016

@jzaefferer @scottgonzalez Updated the branch. Let my know what you think!

@@ -56,7 +56,7 @@ test( "value", function( assert ) {
equal( this.element.calendar( "value" ), "1/1/14", "getter" );

this.element.calendar( "value", "abc" );
equal( this.element.calendar( "value" ), "1/1/14", "Setting invalid values should be ignored." );
equal( this.element.calendar( "value" ), null, "Setting invalid values." );

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 12, 2016

Member

Shouldn't this be assert.equal?

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 12, 2016

Author Member

We are currently using this for assert.equalDate and assert.expect only. Do we want this for all qunit globals?

@@ -646,7 +646,8 @@ return $.widget( "ui.calendar", {
if ( arguments.length ) {
this.valueAsDate( this._parse( value ) );
} else {
return this._format( this.option( "value" ) );
return this.option( "value" ) === null ?

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 12, 2016

Member

How about return this.option( "value" ) === null ? null : ...?

This comment has been minimized.

Copy link
@fnagel

fnagel Oct 12, 2016

Author Member

Done.

@jzaefferer
Copy link
Member

jzaefferer commented Oct 12, 2016

Updates look (mostly) good. Still need to fix the CI-failing test.

@fnagel fnagel force-pushed the fnagel:datepicker-working branch from 0891b50 to bd488f9 Oct 12, 2016
@scottgonzalez
Copy link
Member

scottgonzalez commented Oct 19, 2016

You wan't me to reset the min / max option when trying to change their values to an invalid one? So basically the same functionality for min / max option, we now have for value option and method. Correct?

Correct.

@jzaefferer jzaefferer dismissed their stale review Oct 19, 2016

Probably outdated

@jzaefferer
Copy link
Member

jzaefferer commented Oct 25, 2016

I've looked at the timezone-dependent failure. When running the calendar tests with Sao Paulo timezone, the actual/expected values are only 4 seconds apart:

>> calendar: options - value
>> Message: Active button timestamp
>> Actual: 1463972400000
>> Expected: 1463954400000

> Date(1463972400000)
'Tue Oct 25 2016 12:07:23 GMT+0200 (CEST)'
> Date(1463954400000)
'Tue Oct 25 2016 12:07:27 GMT+0200 (CEST)'

Still not sure what's causing this. If we can't figure it out, we can probably make the test a little less precise. We don't care about the time component here, only the date, right?

@jquerybot jquerybot added CLA: Error and removed CLA: Valid labels Oct 25, 2016
@fnagel
Copy link
Member Author

fnagel commented Oct 25, 2016

We don't care about the time component here, only the date, right?

Correct, calendar and datepicker don't care about time at the moment, but we should keep in mind that timepicker addon is a widely used one.

@fnagel
Copy link
Member Author

fnagel commented Oct 25, 2016

@scottgonzalez Added invalid min / max reset incl. tests

@jzaefferer
Copy link
Member

jzaefferer commented Oct 27, 2016

Correct, calendar and datepicker don't care about time at the moment, but we should keep in mind that timepicker addon is a widely used one.

Let's keep the timestamp then, but update the test to only look at the date, not the time.

@fnagel
Copy link
Member Author

fnagel commented Oct 28, 2016

Let's keep the timestamp then, but update the test to only look at the date, not the time.

So, using our dateequal helper everywhere should help, as we already check for year / month / date only:
https://github.com/fnagel/jquery-ui/blob/datepicker-working/tests/lib/qunit-assert-dateequal.js

@fnagel
Copy link
Member Author

fnagel commented Oct 29, 2016

@jzaefferer @scottgonzalez

Fixed tests by replacing a timestamp compare with a dateEqual assrert.

Ready to merge?

@fnagel
Copy link
Member Author

fnagel commented Oct 29, 2016

CLA error?

this.widget.find( "table button.ui-state-active" ).data( "ui-calendar-timestamp" ),
1463954400000,
assert.dateEqual(
new Date( this.widget.find( "table button.ui-state-active" ).data( "ui-calendar-timestamp" ) / 1000 ),

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Oct 30, 2016

Member

This is odd - why is the timestamp data attribute in a different format than the one accepted by new Date? Shouldn't those match?

This comment has been minimized.

Copy link
@fnagel

fnagel Nov 4, 2016

Author Member

You'r right, both should be milliseconds. Changed,

@jzaefferer jzaefferer dismissed scottgonzalez’s stale review Oct 30, 2016

Only a +1/kudos comment remains

@jzaefferer
Copy link
Member

jzaefferer commented Oct 30, 2016

The new Foundation is still working on a new CLA check. Until that is ready we can ignore the check at least for team members.

@fnagel fnagel force-pushed the fnagel:datepicker-working branch from 34faf02 to fb3841f Nov 4, 2016
Fix travis unit test issue due to timezone difference.
@fnagel fnagel force-pushed the fnagel:datepicker-working branch from fb3841f to 68028e7 Nov 4, 2016
@fnagel
Copy link
Member Author

fnagel commented Nov 4, 2016

@jzaefferer So, merge ready now, right? I would merge master in datepicker and push that right away, ok?

@JSFOwner JSFOwner removed the CLA: Error label Nov 5, 2016
@jzaefferer
Copy link
Member

jzaefferer commented Nov 5, 2016

Yep

@fnagel fnagel merged commit 68028e7 into jquery:datepicker Nov 9, 2016
2 of 3 checks passed
2 of 3 checks passed
jQuery Foundation CLA One author has not signed the CLA
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@jquerybot jquerybot added the CLA: Error label Dec 8, 2016
@fnagel fnagel added CLA: Valid and removed CLA: Error labels Dec 11, 2016
@jquerybot jquerybot added CLA: Error and removed CLA: Valid labels Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.