Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Add mixin unit tests for dob-days #876

Merged
merged 1 commit into from
Dec 16, 2016

Conversation

mkly
Copy link
Contributor

@mkly mkly commented Dec 15, 2016

Changes proposed in this pull request:

  • Add mixin unit tests for dob-days
  • Add bower devDependencies for timekeeper library to handle
    'new Date()`
  • Import timekeeper dependency when build environment is "test"

cc @HospitalRun/core-maintainers

- Add mixin unit tests for dob-days
- Add bower devDependencies for `timekeeper` library to handle
  'new Date()`
- Import `timekeeper` dependency when build environment is "test"
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkly thanks for the PR! Can you explain further why timekeeper is needed?

@mkly
Copy link
Contributor Author

mkly commented Dec 15, 2016

The DOBDays mixin uses new Date() to get the current date today

let today = new Date();

This is used to calculate date differences from the current time
years = today.getFullYear() - birthDate.getFullYear();

This would cause the unit test to fail the next day as each day would make the date diff larger so the assertion would eventually fail in the future.

Because of that we need to stub the Date global to return a predefined date as opposed to actually returning the current date and time.

Sinon can do this with useFakeTimers() http://sinonjs.org/docs/#clock-api and that is what I've used in the past. I elected to use timekeeper as I only needed that one bit of functionality. That said, sinon is probably going to be more robust and may be needed in the future. I have not actually used timekeeper in the past but it looked okay from a quick look. There is a project https://github.com/csantero/ember-sinon that wraps in sinon for use with Ember. I am definitely open to that route instead.

I should add just in case it is not clear that timekeeper is intended to only be included in test builds ember build --environment test and ember test. https://github.com/mkly/hospitalrun-frontend/blob/e0483259cc383f4046d40a8a5f53cefe842a59b9/ember-cli-build.js#L35-L37 but it does add yet another 3rd party dependency and I appreciate the concern of additional maintenance.

The more I think about it, the more I think maybe including sinon instead is a safer approach. Based upon the fact that it is very widely used and much more likely to be well maintained into the future. I was hesitant only because it was a large library, but we could end up needing it for future tests anyway.

Let me know if this makes sense and your thoughts about sinon etc and we'll take it from there.

Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkly I'm fine bringing in timekeeper for now and then moving to Sinon later if it is necessary.

@jkleinsc jkleinsc merged commit 7d489c2 into HospitalRun:master Dec 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants