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

Added Item and List components tests with snapshots and correct icon image checking #117

Closed
wants to merge 6 commits into
base: master
from

Conversation

2 participants
@mlukjanska

mlukjanska commented Oct 21, 2018

Hi,

I've extended a bit work with respect to tests already in place - issue #68, and added the following:

1. Item component tests

1.1 test that snapshot renders correctly for items with death date in the past (for the ones in the future I'd need to add extra logic to handle the randomized phrase that I did not have time as of yet)
1.2 test that correct icon appears for items with death date the past
1.3 test that correct icon appears for items with death date in the future
For 1.2 and 1.3 I needed to add the displayName in Item.atoms - this was required to be able to access the styled-components and do assertions, also I noticed that it was not possible using shallow, had to do full mount - details are here

2. List component tests for snapshot and length

I've also added the test data in fixtures that is being used for cases above.

mlukjanska added some commits Oct 21, 2018

@codyogden

This comment has been minimized.

Owner

codyogden commented Oct 21, 2018

Blowing. My. Mind. Fantastic! We hit 100 products, so I merged #39 which messed up the image file names, but it will be reverted on Nov 1 and I'll be able to merge this PR in at that time. 😮 🙌

@mlukjanska

This comment has been minimized.

mlukjanska commented Oct 21, 2018

@codyogden thanks. Was too late with my changes :D I've noticed that icons are not as expected anymore. In fact working now on some solution to handle icons based on theme :) Will add more commits here soon.

mlukjanska added some commits Oct 25, 2018

Added helper to define and get current theme settings, added snapshot…
… tests for themes, but still need to be manually skipped/enabled when changing the themes
@mlukjanska

This comment has been minimized.

mlukjanska commented Oct 25, 2018

I did some major changes in the testing approach, for themes testing with snapshots could not come up with a better approach than skipping the test of the invalid theme, but I'd assume there should be a better way.
One more thing that might cause snapshots to fail are those phrases marked yellow, which I could not find where those are generated. So maybe merge can still wait a bit :)
image

@@ -0,0 +1,44 @@
// Define themes specifics
export const Themes = {

This comment has been minimized.

@codyogden
@@ -0,0 +1,40 @@
import { format } from 'date-fns';
var now = new Date();

This comment has been minimized.

@codyogden

codyogden Oct 25, 2018

Owner
Suggested change Beta
var now = new Date();
var now = new Date('2018-10-25');

Maybe test from a specific date instead of assumed now. This ensures snapshots with relative date phrasing like "10 months ago" will always remain the same.

@codyogden

This comment has been minimized.

Owner

codyogden commented Oct 25, 2018

@mlukjanska I am very grateful. Incredible work. Thank you! 🥇

The string you highlighted is generated in a component method (below), but it should always remain the same in snapshots since the age of 'dead' products should not change. The phrase is partially generated by date-fns/formatDistance.

Component Method:

getYears() {
const { dateClose, dateOpen } = this.props;
const duration = formatDistance(dateClose, dateOpen);
return (` It was ${duration} old.`);
}

@codyogden codyogden closed this Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment