search

softlayer / sl-ember-components

An Ember CLI Addon that provides a variety of UI components.
http://softlayer.github.io/sl-ember-components
MIT License
114 stars 27 forks source link

Review whether use of timezones in tests is problematic #708

Open notmessenger opened 9 years ago

theoshu commented 9 years ago

This issue came up when our tests used the same time zone (Central/Chicago) as our local ember environment (Houston, same time zone). I modified the tests in an issue that we closed #510.

erangeles commented 8 years ago

@notmessenger taking this.

erangeles commented 8 years ago

@notmessenger https://github.com/softlayer/sl-ember-components/commit/48846f87643dcc0a32de818cc978f47886064049#diff-dc0a450dbe440903080df879b8b42b5b makes timezone required in tests. so we have to use timezones in tests since its a required property.

notmessenger commented 8 years ago

@erangeles The real problem defined by this issue is the work done by Theo in https://github.com/theoshu/sl-ember-components/commit/1482fa061782376c19d0d63baf7a4d2df056b642 which is very brittle. The approach needs to be refactored.

erangeles commented 8 years ago

@notmessenger currently the approach to provide the datetime string is this https://github.com/softlayer/sl-ember-components/blob/EmberCli-1.13.8/addon/components/sl-date-time.js#L123 that approach is similar to http://momentjs.com/timezone/docs/#/using-timezones/parsing-in-zone/ this approach in the docs. What makes the approach brittle? It can be more defensively coded but I don't see any gaps in logic, but I could be wrong.

notmessenger commented 8 years ago

@erangeles https://github.com/softlayer/sl-ember-components/blob/EmberCli-1.13.8/addon/components/sl-date-time.js#L126 uses https://github.com/softlayer/sl-ember-components/blob/EmberCli-1.13.8/addon/components/sl-date-time.js#L92 which is based off of the information of the system the code is currently running in/on, which is perfectly fine for the operation of the code. The problem is that the test is hard-coded to check/expect a specific timezone value when it is unknown which timezone the date value which actually be ran in as we use CI, a dev using the repo is in another timezone, etc (https://github.com/theoshu/sl-ember-components/commit/1482fa061782376c19d0d63baf7a4d2df056b642) This change only made the tests pass against the current CI environment but the approach used to test this functionality is what is flawed. The functionality needs to be tested in a timezone-agnostic way. Perhaps the testing of the format instead is warranted here - like you had to do in a previous issue - but what is being tested will need to be examined to determine wether this approach will be valid or not.

notmessenger commented 8 years ago

@erangeles Could also possibly use https://github.com/plaa/TimeShift-js or just use Sinon and set stub methods on the Date object to return specific response. Prefer the Sinon approach to keep the number of required libraries to a minimum.

notmessenger commented 8 years ago

Moving this issue to different milestone, to be worked alongside https://github.com/softlayer/sl-ember-components/issues/1246