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 test_build_devtools CI job to run test-build-devtools #17631

Merged
merged 6 commits into from Dec 17, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 16, 2019

We're testing the wrong thing in this job. Probably bad copy paste.
As a result, regressions like #17599 creep in.

First commit is a small fix that fixes running it locally after a package got renamed. (The command crashed for me locally otherwise, which made verifying the fix annoying.)

The second commit makes the right command run on CI. It fails, as expected.

The third commit fixes #17630.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6e60342:

Sandbox Source
modest-shirley-we11g Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 16, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9cea553:

Sandbox Source
nameless-wind-ydqrj Configuration

@sizebot
Copy link

sizebot commented Dec 16, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 9cea553

@sizebot
Copy link

sizebot commented Dec 16, 2019

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 386796d

@gaearon gaearon requested a review from bvaughn December 16, 2019 23:21
This should fix the createRoot error.
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -278,7 +278,7 @@ jobs:
- run:
environment:
RELEASE_CHANNEL: stable
command: yarn test-build --maxWorkers=2
command: yarn test-build-devtools --maxWorkers=2
Copy link
Contributor

Choose a reason for hiding this comment

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

lol whoops

? [pathToBabelPluginReplaceConsoleCalls]
: [];
const sourceOnlyPlugins = [];
if (process.env.NODE_ENV === 'development' && !isInDevToolsPackages) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@@ -542,7 +543,7 @@ describe('InspectedElementContext', () => {
// eslint-disable-next-line no-undef
big_int={BigInt(123)}
data_view={dataView}
date={new Date(123)}
date={new Date(exampleDateISO)}
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 16, 2019

OK this one should actually fix it.

@bvaughn
Copy link
Contributor

bvaughn commented Dec 16, 2019

Hm...looks like this has been broken for a while (since the stable/experimental stuff was split out) and we just haven't noticed?

I think to build a version of React that DevTools can consume now we would need to run:

RELEASE_CHANNEL=experimental yarn build -- react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer --type=NODE

(Or just download the latest experimental CI artifact, which might be better.)

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 16, 2019

Do you mean we need to change the CI command to do this too?

@bvaughn
Copy link
Contributor

bvaughn commented Dec 16, 2019

Since DevTools and its tests depend on the createRoot API, which is now only exported for experimental builds, yeah. We'd need to make sure we were feeding it an experimental build.

I think to do this, we'd need to make test_build_devtools depend on the build_experimental target rather than the current build target.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 16, 2019

On CI, we already have test_build_devtools specify build job as a dependency so I don't think there's further action necessary. For local testing it would be nice to do this — or at least to have something warning you when you forget to.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 16, 2019

Since DevTools and its tests depend on the createRoot API, which is now only exported for experimental builds, yeah. We'd need to make sure we were feeding it an experimental build.

This is what I did in 2049b94.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 16, 2019

Ohh I see what you're saying. That this wouldn't be sufficient.

@bvaughn
Copy link
Contributor

bvaughn commented Dec 16, 2019

Yeah, I think to get tests passing we need to use the experimental build command. Then I'll I'll follow up on the rest of this (the actual build/release process) as part of #17629

@bvaughn bvaughn merged commit 36a6e29 into facebook:master Dec 17, 2019
@bvaughn
Copy link
Contributor

bvaughn commented Dec 17, 2019

Thanks Dan! <3

@gaearon gaearon deleted the build-dt branch December 17, 2019 00:04
@gaearon
Copy link
Collaborator Author

gaearon commented Dec 17, 2019

My "pleasure"!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix broken DevTools tests
4 participants