Add storybook ci option to test that Storybook starts "ok"

[virtuoushub]

Test to move away from current "e2e" approach with Cypress for Storybook, and towards leveraging the Storybook CLI tooling.

See:

• add storybook ci option to test start-storybook ok #3141
• fix: check no "ERR!" string is in stderr when running Storybook build #3509 (comment)

[virtuoushub]

@thedavidprice - I am fairly certain where the exit code of 1 is coming from: https://github.com/storybookjs/storybook/blob/f56159989df0c67865fbdf348132d87fba2950ae/lib/core-server/src/build-dev.ts#L99-L111, specifically https://github.com/storybookjs/storybook/blob/f56159989df0c67865fbdf348132d87fba2950ae/lib/core-server/src/build-dev.ts#L108

With this knowledge, I was able to hack around the conditional logic by adding a dummy previewUrl which results in a 0 exit code.

-      ci && '--ci --smoke-test',
+      ci && '--ci --smoke-test --preview-url "ci"',

[thedavidprice]

@virtuoushub definitely looks like you're on to something here! (And, oh my, huge respect for the time it took to dig through SB's source... always an adventure.)

Using this PR code via the GitPod workspace , I ran into a false positive when I try to force an error (surprisingly hard with SB). Here's how you can reproduce:

1. Modify components/BlogPost/BlogPost.stories.tsx to introduce an error (example comes from something I fixed earlier this week):

+ import StoryRouter from 'storybook-react-router'
import BlogPost from './BlogPost'

export const generated = () => {
  return <BlogPost />
}

- export default { title: 'Components/BlogPost' }
+ export default { title: 'Components/BlogPost', decorators: [StoryRouter()] }

2. run yarn rw sb --ci, which will exit 0
3. run both yarn rw sb and yarn rw sb --build, which will both throw

Looking at the source you highlighted, maybe you can also try including --force-build-preview . If no go with that, we could also loop in some folks from Storybook to see if they have any suggestions. (And/or abandon this attempt for now. Will follow your lead.)

[virtuoushub]

@virtuoushub definitely looks like you're on to something here! (And, oh my, huge respect for the time it took to dig through SB's source... always an adventure.)

Using this PR code via the GitPod workspace , I ran into a false positive when I try to force an error (surprisingly hard with SB). Here's how you can reproduce:

1. Modify components/BlogPost/BlogPost.stories.tsx to introduce an error (example comes from something I fixed earlier this week):

+ import StoryRouter from 'storybook-react-router'
import BlogPost from './BlogPost'

export const generated = () => {
  return <BlogPost />
}

- export default { title: 'Components/BlogPost' }
+ export default { title: 'Components/BlogPost', decorators: [StoryRouter()] }

1. run yarn rw sb --ci, which will exit 0
2. run both yarn rw sb and yarn rw sb --build, which will both throw

Looking at the source you highlighted, maybe you can also try including --force-build-preview . If no go with that, we could also loop in some folks from Storybook to see if they have any suggestions. (And/or abandon this attempt for now. Will follow your lead.)

Was able to reproduce the false positive using your steps, thanks for these.

---

notes:

Using --force-build-preview just get's us back to where we started, in that it will produce the false negative on the unmodified code. While it got us closer if we were to introduce the error into our story code (it will exit code 1, and output the build errors similar to yarn rw sb / yarn rw sb --build), it is ultimately not going to work for our use case due to the false negative.

To see what is going on with the warning that is causing the exit code of 1 , I ended up modifying Storybook's build-dev locally ( rw-test-app/node_modules/@storybook/core-server/dist/cjs/build-dev.js )

-    if (previewWarnings.length > 0) _nodeLogger.logger.warn(`preview: ${previewWarnings}`);
+    if (previewWarnings.length > 0) _nodeLogger.logger.warn(`preview: ${JSON.stringify(previewWarnings, null, 2)}`);

this results in:

...
WARN preview: [
WARN   {
WARN     "message": "DefinePlugin\nConflicting values for 'process.env'",
WARN     "details": "'{NODE_ENV: \"development\", NODE_PATH: [], STORYBOOK: \"true\", PUBLIC_URL: \".\"}' !== '\"MISSING_ENV_VAR\"'",
WARN     "stack": "Error: DefinePlugin\nConflicting values for 'process.env'\n    at /workspace/rw-test-app/node_modules/webpack/lib/DefinePlugin.js:573:24\n    at Array.forEach (<anonymous>)\n    at walkDefinitionsForValues (/workspace/rw-test-app/node_modules/webpack/lib/DefinePlugin.js:564:31)\n    at /workspace/rw-test-app/node_modules/webpack/lib/DefinePlugin.js:591:5\n    at Hook.eval [as call] (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:19:10), <anonymous>:291:1)\n    at Hook.CALL_DELEGATE [as _call] (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/Hook.js:14:14)\n    at Compiler.newCompilation (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Compiler.js:1053:26)\n    at /workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Compiler.js:1097:29\n    at Hook.eval [as callAsync] (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:40:1)\n    at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/Hook.js:18:14)\n    at Compiler.compile (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Compiler.js:1092:28)\n    at /workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Watching.js:200:19\n    at _next0 (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1)\n    at eval (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:40:1)\n    at watchRunHook (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack-virtual-modules/lib/index.js:236:13)\n    at Hook.eval [as callAsync] (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:34:1)"
WARN   }
WARN ]
...

I think if we solve for this warning then --smoke-test will work as intended.

[virtuoushub]

It seems https://github.com/redwoodjs/redwood/blob/main/packages/core/config/webpack.common.js#L150-L156 might hold the key to resolving that warning.

#742 looks related to Storybook, but it seems all that did is just shuffle around the above linked code block to allow it to be a shared plugin. I might need some help with webpack as I am a little out of my depth. (cc @peterp)

[thedavidprice]

@virtuoushub nice work here, pulling back the layers. I don't have much time to work on this now, but relaying a couple things that come to mind:

1. It might be helpful to add the --loglevel option to our storybook start and build commands. It takes a arg, but we could just hardcode that to most verbose (so either "normal" or full throttle). If you're interested, maybe add via separate PR so we can get it into main.
2. Ah, Storybook and Webpack, our ol' frenemies. It's possible to debug using rwfw project:sync (if you haven't already). E.g. you could modify the lines you linked and see if it changes --ci outcome. But most of all for now, here's all the Webpack Config related to storybook, which is all merged together.
   • https://github.com/redwoodjs/redwood/blob/5c192358bf90b2ca18fa6f38c79b79c8fccbff00/packages/testing/config/storybook/main.js
   • https://github.com/redwoodjs/redwood/blob/8482ccec00adba5b904b165af6ca3af94336ed66/packages/core/config/webpack.development.js
   • https://github.com/redwoodjs/redwood/blob/d788d7970974dec217a9ae62d5917fc43e176113/packages/core/config/webpack.common.js

My guess is that something from main.js is conflicting. But I'll need to look closer.

[virtuoushub]

@thedavidprice,

@virtuoushub nice work here, pulling back the layers. I don't have much time to work on this now, but relaying a couple things that come to mind:

1. It might be helpful to add the --loglevel option to our storybook start and build commands. It takes a arg, but we could just hardcode that to most verbose (so either "normal" or full throttle). If you're interested, maybe add via separate PR so we can get it into main.

I had tried leveraging --loglevel in the past and it did not yield too much more relevant information. Will definitely give it another shot.

2. Ah, Storybook and Webpack, our ol' frenemies. It's possible to debug using rwfw project:sync (if you haven't already). E.g. you could modify the lines you linked and see if it changes --ci outcome. But most of all for now, here's all the Webpack Config related to storybook, which is all merged together.

   • https://github.com/redwoodjs/redwood/blob/5c192358bf90b2ca18fa6f38c79b79c8fccbff00/packages/testing/config/storybook/main.js
   • https://github.com/redwoodjs/redwood/blob/8482ccec00adba5b904b165af6ca3af94336ed66/packages/core/config/webpack.development.js
   • https://github.com/redwoodjs/redwood/blob/d788d7970974dec217a9ae62d5917fc43e176113/packages/core/config/webpack.common.js

Not sure if something changed, but I was able to run with --ci and exit code 0 ( did not produce any warnings ). However using --ci did not work as expected and resulted in the false positive you originally pointed out. All these findings came from the Gitpod environment, so I will destroy the old one and retry my testing in a new one in case that somehow got in a weird state; however, at this point I think it is best to loop some Storybook folks in as I don't think --ci behaves the way we think, or maybe even as it is intended to (we found a bug)?

My guess is that something from main.js is conflicting. But I'll need to look closer.

[shilman]

Sorry for the slow response on this. Can you please tell me how to run this locally so I can debug the SB side? Thanks! 🙏

[virtuoushub]

Sorry for the slow response on this. Can you please tell me how to run this locally so I can debug the SB side? Thanks! 🙏

No worries @shilman, in terms of locally I might defer to @thedavidprice as to the most up to date how-to. I used the contributing guide myself.

That being said, I have found the easiest way to get this PR running is via - Gitpod and then follow @thedavidprice's steps to reproduce comment

[thedavidprice]

@shilman Huge thank you for taking time to look at this. We got to this point because we're trying to improve our Storybook CI check. And our primary question is:

• does --smoke-test behave as expected in Redwood (i.e. on it's own, it exits with an error 1)
• if it is supposed to exit(1), then how/if can we configure to use in our CI checks?

Note: to see the behavior, you'll need to edit this file:
https://github.com/redwoodjs/redwood/pull/3515/files#diff-817052acd6de5fe2cfea88b164dfb6974f11f6a5a6d0ba6d0ce8fdf9d2c1fb23

L68 ci && '--ci --smoke-test --preview-url "ci"', should be ci && '--ci --smoke-test', We added --preview-url to see if we could get around the exit(1)

👉 Here's a video walkthrough (under 5 mins!) that will get you up and running in a jiffy.

GoogleChrome-26Oct21_20-19-58.mp4

Links to Files

The directories and files I mention in the video:

• Storybook Config: https://github.com/redwoodjs/redwood/tree/main/packages/testing/config/storybook
• RW Storybook CLI Command: https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/storybook.js
• RW Webpack and Babel Config: https://github.com/redwoodjs/redwood/tree/main/packages/core/config

Dev Stuffz

• Contributing Doc: https://github.com/redwoodjs/redwood/blob/main/CONTRIBUTING.md
• RW Storybook commands: see 'em all with yarn rw sb --help
• Sync the changes in Framework code with the Test Project: from the test-project directory, run yarn rwfw project:sync

[thedavidprice]

@shilman gentle nudge

[shilman]

@thedavidprice Sorry your gentle nudge got lost in the infinite sea of notifications & crazy SB 6.4 release. Thanks for the wonderful instructions, I'll take a proper look today!!

[shilman]

Thanks again for your amazing instructions and great DX 😍

Based on a first pass, I think the problem is on the Redwood side. On a standalone project running SB6.3 I did the following

$ yarn storybook --ci --smoke-test
$ echo $?
0

Then I introduced some errors:

• throw new Error('blah') (runtime error)n
  • in main.js => 1
  • in preview.js => 0
• asdf (runtime error)
  • in main.js => 1
  • in preview.js => 0
• asdf\?x12?!sdf (syntax error)
  • in main.js => 1
  • in preview.js => 1

This is all what I would expect. main.js runs in node, so a runtime error would cause the smoke test to fail. preview.js runs in the browser, so the smoke test only fails if you execute a browser, which --smoke-test doesn't do. (NOTE that we will be introducing a new test runner in 6.5 that does a lot more--would love to collaborate on getting this into redwood when the time comes!)

Introducing the throw new Error('blah') to main.js in Redwood does the following:

I took a quick look at the execa call but didn't spend time debugging it. Let me know if you'd like me to dig deeper!

[thedavidprice]

@shilman

(NOTE that we will be introducing a new test runner in 6.5 that does a lot more--would love to collaborate on getting this into redwood when the time comes!)

^^ Saw the news about 6.4 but haven't dug in. Re: 6.5 Would be very excited to talk test runner. We just got v1-rc out with the goal to hit GA in Q1. So after the holidays could be a great time to get our heads together and plot a course 🚀

--

Thank you for taking another look at this. It's definitely hard to parse what's going on because there's layers of config and commands wrapping commands. But mentioning execa gave me an idea about how we can simplify diagnosing this.

Under the hood, here's the command that's run from the web/ directory:

yarn start-storybook --config-dir node_modules/@redwoodjs/testing/config/storybook --port 7910 --no-version-updates --static-dir web/public --ci --smoke-test

Reproduce --smoke-test exit(1)

We can use my test project, currently at the latest version, to run the commands without wrappers.

1. git clone [email protected]:thedavidprice/redwood-tutorial-test.git
2. cd redwood-tutorial-test
3. yarn
4. cd web
5. yarn msw init public --no-save // for mocks; this is run before start-storybook when running "yarn rw storybook"
6. yarn start-storybook --config-dir ../node_modules/@redwoodjs/testing/config/storybook --port 7910 --no-version-updates --static-dir web/public // should run successfully and open browser; stories render successfully
7. yarn start-storybook --config-dir ../node_modules/@redwoodjs/testing/config/storybook --port 7910 --no-version-updates --static-dir web/public --ci --smoke-test // this is what we've been trying to run; should exit(1)
8. yarn start-storybook --config-dir ../node_modules/@redwoodjs/testing/config/storybook --ci --smoke-test // sanity check; still results in exit(1)

The question --> any idea why step 6 works but 7 and 8 exit(1)? 🤷‍♂️

Step 6 (start-storybook) Success

Step 7 and 8 (--smoke-test) exit with erro

[virtuoushub]

@thedavidprice - you might be able to leverage my hack from earlier where we modify node_modules/@storybook/core-server/dist/cjs/build-dev.js like so:

-    if (previewWarnings.length > 0) _nodeLogger.logger.warn(`preview: ${previewWarnings}`);
+    if (previewWarnings.length > 0) _nodeLogger.logger.warn(`preview: ${JSON.stringify(previewWarnings, null, 2)}`);

that way we can better see what is going on with

...
WARN preview: [object Object]
...

and hopefully determine why steps 7 & 8 are exit(1)ing.

[shilman]

@thedavidprice There was a bug in the debugging output. It should have read:

WARN preview: [{"message":"DefinePlugin\nConflicting values for 'process.env'","details": ...

So there are a few things going on here:

1. the debug output needs to be fixed
2. the warning is getting swallowed in dev mode somehow
3. the conflicting values thing is a bug introduced by webpack5 and is fixed in SB6.4

EDIT: @virtuoushub I see you've already gone through all this above. apologies for restating!

[virtuoushub]

@shilman - any advice on how to resolve this:

...
Module not found: Error: Cant resolve 'path' in '/tmp/redwood.du00ic/node_modules/@storybook/store/dist/esm'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
  - add a fallback 'resolve.fallback: { "path": require.resolve("path-browserify") }'
  - install 'path-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
  resolve.fallback: { "path": false }
...

It looks to have started when I bumped to SB6.4

[thedavidprice]

Heroic effort on this one @virtuoushub If you add up all the time we're about to save not having to wait as long on CI to pass, it was very very very worth it.

Thank you again 🚀

[dac09]

Hey @virtuoushub, me again! We have a small regression caused by this change here. Wondering if you could give me a little more context to what this change was for/why?

[thedavidprice]

Without this, the Storbook --ci and --smoke-test flags were failing. (Please correct me if I am wrong.)

FYI, Pete, Danny is going to remove Dotenv entirely over here #4334 Based on all the research you did, any reaction as to whether this removal will have downstream affect on Storybook config? (We are assuming none.)

[virtuoushub]

Hi @dac09 , it was added to avoid:

...
WARN preview: [
WARN   {
WARN     "message": "DefinePlugin\nConflicting values for 'process.env'",
WARN     "details": "'{NODE_ENV: \"development\", NODE_PATH: [], STORYBOOK: \"true\", PUBLIC_URL: \".\"}' !== '\"MISSING_ENV_VAR\"'",
WARN     "stack": "Error: DefinePlugin\nConflicting values for 'process.env'\n    at /workspace/rw-test-app/node_modules/webpack/lib/DefinePlugin.js:573:24\n    at Array.forEach (<anonymous>)\n    at walkDefinitionsForValues (/workspace/rw-test-app/node_modules/webpack/lib/DefinePlugin.js:564:31)\n    at /workspace/rw-test-app/node_modules/webpack/lib/DefinePlugin.js:591:5\n    at Hook.eval [as call] (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:19:10), <anonymous>:291:1)\n    at Hook.CALL_DELEGATE [as _call] (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/Hook.js:14:14)\n    at Compiler.newCompilation (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Compiler.js:1053:26)\n    at /workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Compiler.js:1097:29\n    at Hook.eval [as callAsync] (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:40:1)\n    at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/Hook.js:18:14)\n    at Compiler.compile (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Compiler.js:1092:28)\n    at /workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/lib/Watching.js:200:19\n    at _next0 (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1)\n    at eval (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:40:1)\n    at watchRunHook (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack-virtual-modules/lib/index.js:236:13)\n    at Hook.eval [as callAsync] (eval at create (/workspace/rw-test-app/node_modules/@storybook/builder-webpack5/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:34:1)"
WARN   }
WARN ]
...

See also: #3515 (comment)

@thedavidprice , I think the removal should be safe; 🤞🏻 that the smoke test catches a regression if I am wrong.