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: ensure babel-preset-gatsby can be used with unit tests #9629

Merged
merged 12 commits into from
Nov 5, 2018

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Nov 1, 2018

This PR does a few things, namely:

This PR does a few things, namely:

- Fixes a few things with the test stage, e.g. uses commonjs modules for
test running, uses the current node version for @babel/preset-env, etc.
- Fixes up docs slightly for the unit testing guide
- Fixes gatsbyjs#9614
@DSchau DSchau requested a review from a team November 1, 2018 14:50
Copy link
Contributor Author

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Left a few comments!

@@ -27,31 +27,26 @@ npm install --save-dev jest babel-jest react-test-renderer identity-obj-proxy 'b

Because Gatsby handles its own Babel configuration, you will need to manually
tell Jest to use `babel-jest`. The easiest way to do this is to add a `"jest"`
section in your `package.json`. You can set up some useful defaults at the same
section in your `jest.config.js`. You can set up some useful defaults at the same
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I made a few opinionated changes here, namely package.json -> jest.config.js

@@ -150,11 +145,12 @@ import React from "react"
import renderer from "react-test-renderer"
import Bio from "./Bio"

describe("Bio", () =>
describe("Bio", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is quite a bit clearer/real-world, even though both were valid

@@ -20,7 +20,7 @@ function preset(context, options = {}) {
const stage = process.env.GATSBY_BUILD_STAGE || `test`

if (!targets) {
if (stage === `build-html`) {
if (stage === `build-html` || stage === `test`) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think this makes sense, right? Should transpile to whatever the current version of node is in the unit test runner.

docs/docs/unit-testing.md Outdated Show resolved Hide resolved
@DSchau DSchau dismissed kakadiadarpan’s stale review November 1, 2018 15:00

Addressed the feedback :)

@DSchau
Copy link
Contributor Author

DSchau commented Nov 1, 2018

Also @kentcdodds if you're feeling spicy 🌶, want to take a look at examples/using-jest and make sure it looks good to your eyes? I used react-testing-library for the bulk of the unit testing functionality, so just want to make sure it largely looks solid!

For context, the examples/ directory is just supposed to show real-world illustrations of some concept, e.g. using typescript, an instagram-like app built with Gatsby, and of course, using jest! All the tests are in src/**/__tests__

No worries if not, though :)

module.exports = {
...gatsby,
graphql: jest.fn(),
Link: jest.fn().mockImplementation(({ to, ...rest }) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kinda janky, but I also think that this should probably be added to the docs/unit-testing.md document!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(also just for clarity here, gatsby-link errors out in Jest, so this is a way to side step that issue)

Copy link
Contributor

@shannonbux shannonbux left a comment

Choose a reason for hiding this comment

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

Looks great, just made one note!


Kick off your next Gatsby app with some great testing practices enabled via [Jest][jest], [react-testing-library][react-testing-library], and of course, [Gatsby][gatsby] 💪

Check out the [unit testing doc][unit-testing-doc] for further info!
Copy link
Contributor

Choose a reason for hiding this comment

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

will this link work? Looks...incomplete to me

Copy link
Contributor

Choose a reason for hiding this comment

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

oh and it's mentioned below too, with the complete link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! These are markdown style links, so it's like a table of links that it refers to below. Actually a good practice to get into, I think :) Keeps links in one place so they're easy to edit/re-use!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as an example if it's helpful!

This is a link to [google][google], and [this link][google] will also refer to the google link! 🎉 

[google]: https://google.com

Copy link
Contributor

Choose a reason for hiding this comment

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

ohhhhhh kewl

Copy link
Contributor

Choose a reason for hiding this comment

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

that seems handy. I'll keep that in mind!

@DSchau
Copy link
Contributor Author

DSchau commented Nov 5, 2018

@pieh I think we should take a look at this today. Wanna give it a review? 👀

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

LGTM, just one link need to be adjusted to work on gatsbyjs.org

docs/docs/unit-testing.md Outdated Show resolved Hide resolved
Co-Authored-By: DSchau <DSchau@users.noreply.github.com>
Copy link
Contributor

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

If ya'll want my opinion, I think the example tests have far too many querySelector and querySelectorAll (which are testing implementation details), and too many getByTestId (which are suboptimal). Any chance these could be changed to the better getByText, getByLabelText, getByAltText, etc queries?

@DSchau
Copy link
Contributor Author

DSchau commented Nov 5, 2018

@kentcdodds hey sure, thanks for chiming in! I'll update--I appreciate the feedback!

@pieh pieh merged commit 401df07 into gatsbyjs:master Nov 5, 2018
DSchau added a commit to DSchau/gatsby that referenced this pull request Nov 5, 2018
This (simple) PR fixes a bug I was noticing in gatsbyjs#9629, specifically that
__tests__/*.js files _were_ being included in the bundle.

This is because the isTestFile function in gatsby-plugin-page-creator is
passed the file name, not the file name _with_ the folder (__tests__ in
this case).

I added a second check to check the dir, but could also just implement
this check with the raw file path, as well
@DSchau DSchau deleted the babel-preset-gatsby/modules-fix branch November 5, 2018 20:03
@DSchau
Copy link
Contributor Author

DSchau commented Nov 5, 2018

Successfully published:
 - babel-preset-gatsby@0.1.3

pieh pushed a commit that referenced this pull request Nov 6, 2018
…ctually ignored (#9720)

* fix: ensure that __tests__ directory is actually ignored

This (simple) PR fixes a bug I was noticing in #9629, specifically that
__tests__/*.js files _were_ being included in the bundle.

This is because the isTestFile function in gatsby-plugin-page-creator is
passed the file name, not the file name _with_ the folder (__tests__ in
this case).

I added a second check to check the dir, but could also just implement
this check with the raw file path, as well

* chore: use back ticks

* fix: account for nested directories and switch to micromatch

* chore: split to test windows path matching

* chore: run linter

* chore: add windows path matching test (micromatch supports natively)

* fix: pass single string to isMatch, instead of array

* Revert "fix: pass single string to isMatch, instead of array"

This reverts commit dbc2ffe.
lipis added a commit to lipis/gatsby that referenced this pull request Nov 6, 2018
* 'master' of github.com:gatsbyjs/gatsby: (63 commits)
  Update how-to-contribute.md to mention the style guide when writing blogs/tutorials (gatsbyjs#9742)
  Added  Tylermcginnis website (gatsbyjs#9619)
  Fix grammar and punctuation (gatsbyjs#9498)
  Fix typo of plugin authoring (gatsbyjs#9737)
  Authentication tutorial - small fixes (gatsbyjs#9738)
  chore: move run-sift (gatsbyjs#9549)
  docs: fix minor typo (gatsbyjs#9730)
  chore(release): Publish
  fix(gatsby-plugin-page-creator): ensure that __tests__ directory is actually ignored (gatsbyjs#9720)
  fix: revert admin redirect (gatsbyjs#9728)
  fix: adjust page order to make nested matchPaths work (gatsbyjs#9719)
  feat(gatsby-plugin-sharp): cache base64 if possible (gatsbyjs#9059)
  chore(release): Publish
  fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources (gatsbyjs#9679)
  chore(release): Publish
  fix: ensure babel-preset-gatsby can be used with unit tests (gatsbyjs#9629)
  feat(www): Filter posts by date (gatsbyjs#9400)
  fix(blog): azure blog post url date (gatsbyjs#9718)
  feat(blog): Add post on publishing to Azure (gatsbyjs#8868)
  Emphasize importance of promise return on source-plugin docs (gatsbyjs#9650)
  ...
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…#9629)

This PR does a few things, namely:

- Fixes a few things with the test stage, e.g. uses commonjs modules for
test running, uses the current node version for @babel/preset-env, etc.
- Fixes up docs slightly for the unit testing guide
- Fixes gatsbyjs#9614

<!--
  Q. Which branch should I use for my pull request?
  A. Use `master` branch (probably).

  Q. Which branch if my change is a bug fix for Gatsby v1?
  A. In this case, you should use the `v1` branch

  Q. Which branch if I'm still not sure?
  A. Use `master` branch. Ask in the PR if you're not sure and a Gatsby maintainer will be happy to help :)

  Note: We will only accept bug fixes for Gatsby v1. New features should be added to Gatsby v2.

  Learn more about contributing: https://www.gatsbyjs.org/docs/how-to-contribute/
-->
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…ctually ignored (gatsbyjs#9720)

* fix: ensure that __tests__ directory is actually ignored

This (simple) PR fixes a bug I was noticing in gatsbyjs#9629, specifically that
__tests__/*.js files _were_ being included in the bundle.

This is because the isTestFile function in gatsby-plugin-page-creator is
passed the file name, not the file name _with_ the folder (__tests__ in
this case).

I added a second check to check the dir, but could also just implement
this check with the raw file path, as well

* chore: use back ticks

* fix: account for nested directories and switch to micromatch

* chore: split to test windows path matching

* chore: run linter

* chore: add windows path matching test (micromatch supports natively)

* fix: pass single string to isMatch, instead of array

* Revert "fix: pass single string to isMatch, instead of array"

This reverts commit dbc2ffe.
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.

"SyntaxError: Unexpected string" when implementing unit testing guide on stock blog starter
5 participants