Skip to content

Conversation

@christianalfoni
Copy link
Contributor

@christianalfoni christianalfoni commented Oct 15, 2019

Allows you to easily test a PR:

Screenshot 2019-10-15 at 10 03 16

Note the number of the PR, in this example: 2787

In the root of client run: yarn test:pr 2787

It will automatically checkout the PR and update and build the deps. This is just to ensure the environment is correct related to testing.

If you want to make changes you can do so by:

  • Just push the automatically created branch to Github as normal
  • Create a new PR, but choose the existing PR (2787) as the source
  • This creates a PR on the repo of the contributor and the contributor has to merge in this change, so write a comment on the original PR pointing to what needs to be merged in

I think this creates a really good collaborative feeling for the contributor. We are helping them helping us, instead of overriding what they are doing.

@vercel
Copy link

vercel bot commented Oct 15, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/codesandbox/codesandbox-client/5jsxr6tfr
🌍 Preview: https://codesandbox-client-git-testpr.codesandbox1.now.sh

testpr-script.js Outdated
const branchName = `pr-${username.sync()}-${prId}`;

exec(
`git fetch origin pull/${prId}/head:${branchName}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

@christianalfoni This wont work on forks I guess? 🤔

Copy link
Contributor Author

@christianalfoni christianalfoni Oct 15, 2019

Choose a reason for hiding this comment

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

It is for forks :) Take a look at: #2787 , the comments

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean if I run locally yarn test:pr 2787 this wont work, since my origin is my own fork of the repo and my upstream is the original repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see :) Yeah, the intention was that the devs who merge into master has a chance to verify the PR before they do so :)

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's only for CodeSandbox-people, my comment is obsolete 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not want to use the term Codesandbox-people, hehe... "merge people" 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Same people, different name 😉

@vercel vercel bot temporarily deployed to staging October 15, 2019 08:40 Inactive
@lbogdan lbogdan temporarily deployed to pr2790 October 15, 2019 10:32 Inactive
testpr-script.js Outdated
.then(() => spawnPromise('yarn', ['install']).then(() => {
return spawnPromise('yarn', ['build:deps']);
}))
.then(() => spawnPromise('yarn', ['build:deps']))
Copy link
Contributor

Choose a reason for hiding this comment

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

But what if they are changed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You will get an error either building when you start the project or when building the deps :) Just as you normally do

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.

5 participants