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

test(e2e): add stderr check to Storybook build #3508

Conversation

virtuoushub
Copy link
Collaborator

@virtuoushub virtuoushub commented Oct 5, 2021

In local testing, yarn rw storybook --build returned with a 0 exit code, however there was stderr.

Although this looks to be fixed in #3154, in certain circumstances the command results in no Storybook artifacts being built (do not have the specific error stacktrace offhand); which in turn, causes subsequent portions of the Cypress test to fail due to lack of the Storybook assets it is expecting to check the existence of. This was not clear in the Cypress output.

This newly add stderr check should surface stderrtake care of this case and avoid any future false negatives when calling the storybook build in this way.


Additional flavor

Noticed this while looking into a failure of another PR I was working on.

See also:

In local testing, `yarn rw storybook --build` returned with a `0` exit code, however there was `stderr`.

This results in no Storybook artifacts being built, which in turn causes subsequent portions of the Cypress test to fail due to lack of the Storybook assets it is expecting to check the existence of.

This newly add `stderr` check should take care of this case and avoid any future false negatives when calling the storybook build in this way.

---

*Additional flavor*

Noticed this while looking into a failure of [another PR I was working on](https://github.com/redwoodjs/redwood/actions/runs/1299581375).

See also: https://discord.com/channels/679514959968993311/716252919875240007/894760524095578205
@thedavidprice
Copy link
Contributor

Thanks for adding this @virtuoushub Our use case for Cypress is basically a hack — it's not really intended for this. So we stumble over issues like this occasionally, which are effectively due to hitting resource constraints in the CI container. Complete overhaul to come once we get to v1-rc. For now, we stumble forward.

I'm going to merge this now, but want to point out what's happening here:

  • This does console the stderr, but it's not actually the error. It's the preceding console from Storybook captured by Cypress before it failed the spec. In this case, it's a wall of text that actually shows Storybook did build (sigh):
// final output from stderr
[webpack.Progress] 100% \n\ninfo => Output directory: /tmp/redwood.d7Y9Zk/web/public/storybook
  • This E2E test is succeeding locally. And I confirmed Storybook is fine in the latest canary
  • We are about to improve the CI indirectly via this PR that upgrades to Yarn v3. Everything is passing there, so once merged tomorrow 🤞 we'll get past this hurdle.

NOTE TO MAINTAINERS: if you see this test failing in CI but all other checks are passing, just loop me in and we can force merge.

@virtuoushub
Copy link
Collaborator Author

@thedavidprice - Using the output from this PR's first failed CI run, I wrote a better check over at #3509


...which are effectively due to hitting resource constraints in the CI container.

In my local testing it was more likely due to an odd webpack build error (something like "DefinePlugin mainValue.add is not a function"). This is probably the natural result of the complexities of module/dependency resolution using Node/package managers such as yarn (npm is also not immune to problems such as this [and makes sense why #3154 solves it]).

Glad to hear #3154 seems to have fixed the underlying issue(s).

The benefit of this PR / #3509 is that potential root causes are surfaced quicker, instead of relying on very specific (i.e. brittle) dependency combinations.

@thedavidprice
Copy link
Contributor

Glad to hear #3154 seems to have fixed the underlying issue(s).

^^ Wouldn't you know as soon as I wrote that an updated from main... Failing. Ha!

I'll follow up I the other PR. Thanks for keeping on keeping on.

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.

2 participants