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

Add Snapshot testing #194

Merged
merged 7 commits into from
Nov 20, 2020
Merged

Add Snapshot testing #194

merged 7 commits into from
Nov 20, 2020

Conversation

DawoodShahat
Copy link
Contributor

@DawoodShahat DawoodShahat commented Nov 14, 2020

This PR fixes #117

Pull Request checklist

  • The pull request has a descriptive title (and a reference to an issue it
    fixes, if applicable)
  • All tests and linter checks are passing
  • The pull request is free of merge conflicts

@DawoodShahat DawoodShahat marked this pull request as draft November 14, 2020 23:58
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

This looks great @DawoodShahat! I have some concerns about modifying the svelte files, but other than that this is looking very close.

Checking the CI logs, I noticed this warning:

    console.warn
      <FilterInput> was created without expected prop 'filterText'



      at new FilterInput (src/components/FilterInput.svelte:132:12)
      at create_fragment (stories/FilterableList.svelte:56:16)
      at init (node_modules/svelte/internal/index.js:1465:37)
      at new FilterableList (stories/FilterableList.svelte:193:3)
      at getRenderedTree (node_modules/@storybook/addon-storyshots/dist/frameworks/svelte/renderTree.js:20:5)
      at node_modules/@storybook/addon-storyshots/dist/test-bodies.js:11:22
      at Object.<anonymous> (node_modules/@storybook/addon-storyshots/dist/api/snapshotsTestsTemplate.js:42:20)

It looks like maybe the story is lacking something? It would be good if we could make issues like that fail the CI, as they often indicate bugs.

src/components/FilterInput.svelte Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
<script>
import throttle from "just-throttle";
const throttle = require("just-throttle");
Copy link
Contributor

Choose a reason for hiding this comment

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

With lodash we can just do:

Suggested change
const throttle = require("just-throttle");
import { throttle } from "lodash";

... and it'll work

@@ -1,5 +1,5 @@
<script>
import marked from "marked";
const marked = require("marked");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const marked = require("marked");
import { parseInline } from "marked";

@DawoodShahat
Copy link
Contributor Author

Here is what I updated upon @wlach review:

  • Removed just-throttle dependency and instead used _throttle from lodash
  • Fixed the warning that was generated by FilterInput.svelte component
  • Used ES6 imports instead of CJS require

The CI will now fail when there is an unintended change in the markup. I think we should update the README and add some instructions on how to update the generated snapshots with your intended changes e.g. npm run test:jest -- -u.

Also one more thing, What do you think about deploying Stories to netlify?

By the way thanks for the review, I spent a good amount of time on the error.

@DawoodShahat DawoodShahat marked this pull request as ready for review November 19, 2020 00:55
@DawoodShahat DawoodShahat requested a review from wlach November 19, 2020 00:55
@wlach
Copy link
Contributor

wlach commented Nov 19, 2020

The CI will now fail when there is an unintended change in the markup. I think we should update the README and add some instructions on how to update the generated snapshots with your intended changes e.g. npm run test:jest -- -u.

Great idea, could you add something about this to the README? I think we can add them to the storybook: https://github.com/mozilla/glean-dictionary#storybook

Also one more thing, What do you think about deploying Stories to netlify?

This is also a great idea! I think we could probably publish a static copy to /storybook/. I would obviously do this in a followup.

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Code looks great, I think we just need an update to the README and we'll be good to land.

@DawoodShahat DawoodShahat requested a review from wlach November 19, 2020 17:10
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Great stuff, just have some minor suggested changes to the README which I'll apply before landing.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@wlach wlach merged commit a23bd53 into mozilla:main Nov 20, 2020
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.

Run storybook tests in ci
2 participants