-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 image snapshots setup in official-storybook #8932
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import puppeteer, { Browser, Page } from 'puppeteer'; | ||
import { Browser, Page } from 'puppeteer'; | ||
import { toMatchImageSnapshot } from 'jest-image-snapshot'; | ||
import { logger } from '@storybook/node-logger'; | ||
import { constructUrl } from './url'; | ||
|
@@ -21,6 +21,8 @@ const defaultConfig: ImageSnapshotConfig = { | |
getGotoOptions: noop, | ||
customizePage: asyncNoop, | ||
getCustomBrowser: undefined, | ||
setupTimeout: 15000, | ||
testTimeout: 15000, | ||
}; | ||
|
||
export const imageSnapshot = (customConfig: Partial<ImageSnapshotConfig> = {}) => { | ||
|
@@ -33,6 +35,8 @@ export const imageSnapshot = (customConfig: Partial<ImageSnapshotConfig> = {}) = | |
getGotoOptions, | ||
customizePage, | ||
getCustomBrowser, | ||
setupTimeout, | ||
testTimeout, | ||
} = { ...defaultConfig, ...customConfig }; | ||
|
||
let browser: Browser; // holds ref to browser. (ie. Chrome) | ||
|
@@ -75,19 +79,22 @@ export const imageSnapshot = (customConfig: Partial<ImageSnapshotConfig> = {}) = | |
|
||
expect(image).toMatchImageSnapshot(getMatchOptions({ context, url })); | ||
}; | ||
testFn.timeout = testTimeout; | ||
|
||
testFn.afterAll = () => { | ||
testFn.afterAll = async () => { | ||
if (getCustomBrowser && page) { | ||
return page.close(); | ||
await page.close(); | ||
} else if (browser) { | ||
await browser.close(); | ||
} | ||
|
||
return browser.close(); | ||
}; | ||
|
||
testFn.beforeAll = async () => { | ||
const beforeAll = async () => { | ||
if (getCustomBrowser) { | ||
browser = await getCustomBrowser(); | ||
} else { | ||
// eslint-disable-next-line global-require | ||
const puppeteer = require('puppeteer'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to try TS dynamic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it work in Node 10? |
||
// add some options "no-sandbox" to make it work properly on some Linux systems as proposed here: https://github.com/Googlechrome/puppeteer/issues/290#issuecomment-322851507 | ||
browser = await puppeteer.launch({ | ||
args: ['--no-sandbox ', '--disable-setuid-sandbox', '--disable-dev-shm-usage'], | ||
|
@@ -97,6 +104,8 @@ export const imageSnapshot = (customConfig: Partial<ImageSnapshotConfig> = {}) = | |
|
||
page = await browser.newPage(); | ||
}; | ||
beforeAll.timeout = setupTimeout; | ||
testFn.beforeAll = beforeAll; | ||
|
||
return testFn; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
export const version = '5.3.0-beta.3'; | ||
export const version = '5.3.0-beta.5'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import globBase from 'glob-base'; | ||
import { makeRe } from 'micromatch'; | ||
|
||
const isObject = val => val != null && typeof val === 'object' && Array.isArray(val) === false; | ||
export const toRequireContext = input => { | ||
switch (true) { | ||
case typeof input === 'string': { | ||
const { base, glob } = globBase(input); | ||
const regex = makeRe(glob) | ||
.toString() | ||
// webpack prepends the relative path with './' | ||
.replace(/^\/\^/, '/^\\.\\/') | ||
.replace(/\?:\^/g, '?:'); | ||
|
||
return { path: base, recursive: glob.startsWith('**'), match: regex }; | ||
} | ||
case isObject(input): { | ||
return input; | ||
} | ||
|
||
default: { | ||
throw new Error('the provided input cannot be transformed into a require.context'); | ||
} | ||
} | ||
}; | ||
|
||
export const toRequireContextString = input => { | ||
const { path: p, recursive: r, match: m } = toRequireContext(input); | ||
return `require.context('${p}', ${r}, ${m})`; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 how does this work in Jest? I didn't think
require.context()
existed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/storybookjs/storybook/tree/next/addons/storyshots/storyshots-core#configure-jest-to-work-with-webpacks-requirecontext
In our own setup, it's Option 1: Plugin (
babel-plugin-require-context-hook
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right but this isn't universal. For instance:
main.js
entrypoint.What was the issue with what was there before w/ more typical
require()
s?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it is unlikely that user's code is going to run their babel plugins over files in
node_modules
. Or is all this being compiled when we compileaddon/storyshots
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is but require-context plugin isn't applied right now, I'll add that, thanks for noting.
UPD: it's tsc not babel, so I think I'll just import
babel-plugin-require-context-hook/register
and useglobal.__requireContext
directlyWe used different libraries for turning globs into files in
core
andstoryshots
which sometimes led to differences. For instance, "regex or" patterns (.stories.(js|tsx|mdx)
) that we use inmain.js
aren't supported bynode-glob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense.