Time Spec occasionally fails

[aaron-m-edwards]

We need to fix the time on the spec for the Time component. There is a small chance it fails with the following error

 FAIL  src/framework/components/Time/Time.spec.js
  ● DateTimeDisplay component › When rendered without a [format] property › it should display the current time formatted as {HH:mm:ss}

    expect(string).toContain(value)

    Expected string:
      "Time: 18:31:45"
    To contain value:
      "18:31:46"

      at Object.<anonymous> (src/framework/components/Time/Time.spec.js:25:29)
          at Promise (<anonymous>)
      at Promise.resolve.then.el (node_modules/p-map/index.js:42:16)
          at <anonymous>

[sinan-aumarah]

Yeah. We're using reactjs-moment so I'll just change the tests to make sure we're passing the properties correctly to it instead of doing proper DOM rendering. Will be fixed soon :)

[sinan-aumarah]

Fixed.

[aaron-m-edwards]

Cheers, I would have done it myself but I am half way though a spike on how we could possibly clean up some of the event handling/button code using a higher order component and I didn't want to loose my focus

[sinan-aumarah]

It's all good mate :) I'll work on increasing the unit test coverage

[aaron-edwards]

Do you want to jump on slack. I have a couple of suggestions about unit tests.

[sinan-aumarah]

Yeah sure. I'll be there soon :)

[watsonarw]

Hmm, just saw this. I thought I fixed it with https://github.com/twlevelup/watch_edition_react/commit/5098b8662b3bbe0f3a49f8381297c7fd0a030860

[sinan-aumarah]

@watsonarw I think it used to fail when the time difference between mount(..component) and assertion is off by few milliseconds.

const result = mount(<Time format='HH-HH-mm-ss' />).find('.clock-time');    -> 5.59735345 seconds
       const currentHourAndMinutes = moment().format('HH-HH-mm-ss');    -> 6 seconds

I think if we really want to test that component we have to mock moment but I don't see the point since we're trusting reactjs-moment to do that for us.

[watsonarw]

That's what it looked like to me too, hence locking the time before each test, and releasing it afterwards. Good call on not really needing to test the third party library though.

[aaron-m-edwards]

The problem with 5098b86 was in the order of execution in jest.

describe('DateTimeDisplay component', () => {
beforeEach(() => {
     MockDate.set(moment());
   });

   afterEach(() => {
     MockDate.reset();
   });

    const TimeWrapper = mount(<Time />));
...
});

Would

1. Source the describe
   • Jest will add all tests to a list to execute later
   • At this point it creates the TimeWrapper by mounting the time component
2. Run tests
   • At this point the beforeEach's will be run, in this case mocking moment.
   • Then tests run, however it will test on the mounted component sourced in step 1, which could be a while ago

An alternate solution would be to move the mounting of to inside the beforeEach function

[watsonarw]

I'd say we should probably be doing that anyway... Having a constant that persists between different test cases is just asking to have test execution order dependent issues.

[aaron-m-edwards]

That was one of the things I was talking about to @sinan-aumarah on slack. There are a bunch of enzyme wrappers (as well as maps) which are being bound to consts in test. It is fine to do that to immutable stuctures (numbers/strings etc) just nothing that is mutable