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.

Sign up for GitHub

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

Jump to bottom

Date: Create unit tests to verify day parsing uses the correct day range given its corresponding month/year #374

Closed
wants to merge 8 commits into from
Closed

Date: Create unit tests to verify day parsing uses the correct day range given its corresponding month/year #374

wants to merge 8 commits into from

Conversation

rxaviers
Copy link
Member

Fixes #323
Ref #373
@rxaviers
Copy link
Member Author

In order to properly test the #323 fix (PR #373), I needed to overwrite Date, so an arbitrary "today" could be set. Therefore, I came up with FakeDate, which has a non-trivial implementation. So, it would be good to have more eyeballs reviewing this code (cc @jzaefferer, @scottgonzalez) in an attempt to simplify it where possible.
@rxaviers rxaviers mentioned this pull request Dec 30, 2014
Closed
@scottgonzalez
Copy link
Contributor

Why not just use Sinon.JS? http://sinonjs.org/
@jzaefferer
Copy link
Contributor

Specifically the clock feature: http://sinonjs.org/docs/#clock

var clock = sinon.useFakeTimers(now);

Where now is your custom "today" as a timestamp. That should do the trick.
rxaviers and others added 8 commits January 12, 2015 10:24
@rxaviers
Bower: Sort dependencies
59b3195
@mateubo @rxaviers
Date: Fix parsing of the day part when it precedes the month part
1e23b94 
Fixes #323
@mateubo @rxaviers
Date: Fix parsing of the day part when it precedes the month part
c602794 
Fixes #323
@mateubo @rxaviers
Date: Improved verification of the value for the D token being with…
ba9cf68 
…in the correct range by taking into account leap years
@mateubo @rxaviers
Date: Improved verification of the value for the D token - fixed a …
83b18db 
…typo
@mateubo @rxaviers
Date: renamed totalDays to daysOfYear
1c46da4
@rxaviers
Test: Day parsing must use the correct day range
9351898 
Create unit tests to verify day parsing uses the correct day range given
its corresponding month/year.

Fixes #323
Closes #373
@rxaviers
squash! Test: Day parsing must use the correct day range
9e870a3 
lolex's 3664eb8d90dd1d6fa04a098f77b77b19a6bb5850 = 1.1.0 (present on npm,
but not git tagged accordingly).

Create unit tests to verify day of year parsing uses the correct day range given
its corresponding year.
@rxaviers
Copy link
Member Author

Awesome. I've updated this PR to use Sinon.js instead of my custom implementation on tests.
@jzaefferer
Copy link
Contributor

Looks good to me.
@rxaviers
Copy link
Member Author

Testing this PR+Sinon.js across various browsers resulted in errors/failures (I've merged in the browserstack-runner branch locally to test it). Running browserstack-runner against my custom implementation passes just fine.

  • Sinon.js on browserstack: fail.
  • Custom FakeDate on browserstack: pass.

Potentially, the Sinon.js failures are only related to AMD loading, giving the fact some failing tests intermittently pass.
@rxaviers
Copy link
Member Author

We could land this PR + Sinon.js now with no problem. But, it would impact (i.e. delay) testing automation #235 (i.e., to land browserstack-runner branch #367), since we would have to figure out what this failures were.

Giving test automation (which is due to 1.0.0) and #323 have higher priorities compared to using Sinon.js to simplify FakeDate here on tests, I'm considering to track that as a separate issue unless anyone has a different idea. So, we can land #373 (fixing #323), land browserstack-runner branch. Then, spend time on it.

What do you think?
@jzaefferer
Copy link
Contributor

Is the sinon/browserstack failure consistent? Can you reproduce it when running the given browser manually?

If we're not getting anywhere, I'm okay with keeping FakeDate for now.
@rxaviers
Copy link
Member Author

It's not consistent. I haven't tried it manually. The problem could be simple. But, it will require some time investigating it. I just think this time is best spent somewhere else now. :)
@jzaefferer
Copy link
Contributor

Okay, let's give sinon another try later.
@rxaviers rxaviers closed this in 134419f Jan 12, 2015
@rxaviers rxaviers deleted the fix-323 branch January 12, 2015 18:42
@rxaviers rxaviers mentioned this pull request Jan 12, 2015
Open
rxaviers added a commit that referenced this pull request Jan 12, 2015
@rxaviers
Test: Use Sinon.js instead of FakeDate
068f72b 
Note: lolex's 3664eb8d90dd1d6fa04a098f77b77b19a6bb5850 sha corresponds
to its 1.1.0 npm version (version is present on npm, but not git tagged
accordingly).

Ref #374
Fixes #382
@rxaviers rxaviers mentioned this pull request Jan 12, 2015
Closed
ashensis pushed a commit to ashensis/globalize that referenced this pull request Mar 17, 2016
@parndt
Merge pull request globalizejs#374 from hube/fix-fallbacks
6d8f550 
remove usage of Thread.current for fallbacks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants
@rxaviers @scottgonzalez @jzaefferer @mateubo