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

Fix time and date tests in Time.test.js #522

Merged
merged 1 commit into from
Nov 1, 2020
Merged

Fix time and date tests in Time.test.js #522

merged 1 commit into from
Nov 1, 2020

Conversation

mtsknn
Copy link
Contributor

@mtsknn mtsknn commented Oct 31, 2020

I noticed that the current test "should render date and time correctly" is not actually testing that the date and time are formatted correctly. It's just testing that the input and output are the same because the expected values in Time.test.js are specified the same way as in Time.vue:

/* Time.vue */

// Calculate time in 12-hour format
const time = today.toLocaleTimeString([], {
  hour: '2-digit',
  minute: '2-digit',
});
this.computedTime = time;

// Compute date according to the current locale
const date = today.toLocaleDateString([], {
  year: 'numeric',
  month: 'long',
  day: 'numeric',
});
this.computedDate = date;
/* Time.test.js */

expect(wrapper.find('.time').text()).toBe(
  today.toLocaleTimeString([], {
    hour: '2-digit',
    minute: '2-digit',
  }),
);

expect(wrapper.find('.date').text()).toBe(
  today.toLocaleDateString([], {
    year: 'numeric',
    month: 'long',
    day: 'numeric',
  }),
);

So the tests are basically like this:

expect(
  today.toLocaleTimeString([], {
    hour: '2-digit',
    minute: '2-digit',
  })
).toBe(
  today.toLocaleTimeString([], {
    hour: '2-digit',
    minute: '2-digit',
  })
);

// + date test

Or basically like expect(foo()).toBe(foo()).

Testing that the input is the same as the output doesn't give any confidence/certainty that the values are formatted correctly.

Plus what if today.toLocaleTimeString(/* ... */) returned undefined for some reason? Then the test would be basically this:

expect(undefined).toBe(undefined);

That's not quite what we want to test, right? 🙂

In this PR I'm explicitly defining that the expected values are '01:23 PM' and 'March 10, 2020'. This way the tests will actually fail if the time and date are not correctly formatted.


I also removed an unnecessary line from Time.vue.

Copy link
Owner

@jayehernandez jayehernandez left a comment

Choose a reason for hiding this comment

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

Great work! I admit I'm not well-versed in testing myself, so your PR is of great help!

@jayehernandez
Copy link
Owner

References #124

@jayehernandez jayehernandez merged commit 6d76db6 into jayehernandez:master Nov 1, 2020
@jayehernandez
Copy link
Owner

@all-contributors please add @mtsknn for testing

@allcontributors
Copy link
Contributor

@jayehernandez

I've put up a pull request to add @mtsknn! 🎉

@bobsany16
Copy link
Contributor

@mtsknn wow! I learned a lot from this since I wrote the tests 😅. Thanks so much! I tried to mock date before but I just couldn't make it work. However, i learned a new thing today!

@mtsknn
Copy link
Contributor Author

mtsknn commented Nov 1, 2020

@bobsany16, I'm so glad to hear that, thanks! 😊

@mtsknn mtsknn deleted the fix-time-test branch November 1, 2020 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants