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 recipe test problems #23347

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Fix recipe test problems #23347

merged 1 commit into from
Apr 23, 2020

Conversation

freiksenet
Copy link
Contributor

@freiksenet freiksenet commented Apr 21, 2020

Description

  • Strip ansi colors on diff during tests
  • Use tmpdir to run fixtures
  • Fix snapshots having wrong fixture formatting

* Strip ansi colors on diff during tests
* Use tmpdir to run fixtures
* Fix snapshots having wrong fixture formatting
@freiksenet freiksenet requested review from a team as code owners April 21, 2020 12:59
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Thanks for this! Much better solution. TIL about .jestSetup.js.


describe(`gatsby-plugin resource`, () => {
let tmpDir
Copy link
Contributor

Choose a reason for hiding this comment

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

What problems was this causing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test run modified fixtures without restoring them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this is the result of prettier resolving a different config (or none at all) so writing back out the config after the last destroy results in a diff.

Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Probably would not have added tmp-promise for it, but it's only for tests so sure why not :)

Thanks for fixing this!

@@ -18,7 +18,7 @@ Object {
module.exports = {
/* Your site config here */
plugins: [],
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What's up with the semi's in this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably cause we run prettier on the gatsby-config.js source after we add a plugin — so without a .prettierrc around for tests, it adds semis by default

@@ -22685,11 +22685,6 @@ sudo-prompt@^8.2.0:
resolved "https://registry.yarnpkg.com/sudo-prompt/-/sudo-prompt-8.2.5.tgz#cc5ef3769a134bb94b24a631cc09628d4d53603e"
integrity sha512-rlBo3HU/1zAJUrkY6jNxDOC9eVYliG6nS4JA8u8KAshITd07tafMc/Br7xQwCSseXwJ2iCcHCE8SNWX3q8Z+kw==

sudo-prompt@^8.2.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

const diffText = diff(oldVal, newVal, options)
let diffText = diff(oldVal, newVal, options)

if (process.env.GATSBY_RECIPES_NO_COLOR) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm wondering if we could do this in the test instead, and not in code? Or do we want people to have this option too?

Copy link
Contributor

Choose a reason for hiding this comment

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

like pass it in as an option into the function? That wouldn't keep it any more hidden.

Copy link
Contributor

@johno johno left a comment

Choose a reason for hiding this comment

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

💖

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Apr 23, 2020

Doesn't seem like there's any objections to the changes & several people approved & we need to get more recipes changes merged in so getting this in.

Thanks @freiksenet and others for figuring this out!

@KyleAMathews KyleAMathews merged commit 612120f into master Apr 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-recipe-test branch April 23, 2020 21:47
pvhee added a commit to marzeelabs/gatsby that referenced this pull request Apr 24, 2020
* 'master' of github.com:gatsbyjs/gatsby: (64 commits)
  Fix recipe test problems (gatsbyjs#23347)
  create blog post announcing extended deadline for Virtual Gatsby Days… (gatsbyjs#23430)
  Correct CFP deadline date on Virtual Gatsby Days registration announcement (gatsbyjs#23432)
  fix(gatsby-recipes): Raise an error when an unknown resource is used (gatsbyjs#23428)
  feat(gatsby-recipes): While apply a step, show the time elapsed after 10 seconds (gatsbyjs#23362)
  markdownASTToHTMLAst isn't async (gatsbyjs#23427)
  Be more vocal about not doing type checking (gatsbyjs#23397)
  docs(gatsby-remark-images): mark `sizeByPixelDensity` as deprecated (gatsbyjs#23387)
  chore(all): Improve renovate (gatsbyjs#23411)
  chore(gatsby): count sift hits in telemetry (gatsbyjs#23416)
  chore(showcase): add GeneOS and COVID KPI (gatsbyjs#23405)
  feat(analytics): defer google analytics script (gatsbyjs#22806)
  docs: mention passing the .tsx file to createPage (gatsbyjs#23329)
  fix(www): tweak docsearch to init algolia when tabbed into (gatsbyjs#23040)
  chore(docs): Fix typo in url (gatsbyjs#23394)
  chore(gatsby-preset-gatsby-package): Remove tsconfig.json (gatsbyjs#23388)
  fix(gatsby-recipes): link to the raw gist of .estlintrc.js (gatsbyjs#23390)
  docs: Create gitlab-continuous-integration.md (gatsbyjs#23367)
  chore(doc): switch zeit now to Vercel Now for Gatsby deployment (gatsbyjs#23346)
  chore(showcase): add Resume on the Web (gatsbyjs#23371)
  ...
@freiksenet
Copy link
Contributor Author

Lol, didn't realize we didn't merge it before.

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.

6 participants