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

review: test: Migrate FilterTest to Junit5 #3943

Closed
wants to merge 0 commits into from
Closed

review: test: Migrate FilterTest to Junit5 #3943

wants to merge 0 commits into from

Conversation

Rohitesh-Kumar-Jain
Copy link
Contributor

#3919

This PR just migrates FilterTest to Junit5, pure refactoring.

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

Hi @Rohitesh-Kumar-Jain,

Fix looks very clean, nicely done! There are no problems with your fixes, but there is a workflow problem here: you've made a PR from your own master branch. Unfortunately, I missed your question in the previous PR about which branch you should push your changes to, so we'll deal with that now.

In general, for each PR you create a new branch and push to that. You should never push your patches to your own master branch, because then you'll go out of sync with the upstream (Spoon). When I want to make a new PR to Spoon (or any project), my workflow looks like this:

Synchronize the default branch (usually master or main) from the upstream

The upstream is the original repo, so in this case INRIA/Spoon. Its default branch (master) is the ground truth for Spoon, and you should always keep your own fork's default branch synchronized with it. To synchronize with upstream, you need to do the following:

  1. Add an upstream remote
    • git remote add upstream https://github.com/INRIA/Spoon
    • You only need to do this once!
  2. Pull changes
    • git switch master - checkout to master
      • If using an old version of Git (<2.23), you need to use checkout instead of switch
    • git pull upstream master - pull changes to your local repo
    • git push origin master - push changes to your fork on GitHub

With that, your local repository as well as your fork are synchronized with the upstream repo. Now we can turn to creating a new branch for the patch. However, you've "messed up" the state of your master branch, and so we need to do another thing to fix it, more on that at the end of this very long comment.

Create a new branch

I'm sure you know how to create a new branch, but I'll just show you anyway.

  1. Create the branch locally and perform changes
    • git switch -c migrate-filtertest-to-junit5
  2. Perform and commit your changes
  3. Push to a remote branch on your fork
    • git push -u origin migrate-filtertest-to-junit5

Now, you've got a new branch on your fork ready to be the head branch of a pull request.

Create a PR from the new branch

You know this step!

How to fix your current master branch?

First of all, checkout to your master branch, and then checkout to a new PR branch based on that.

git switch master
git switch -c migrate-filtertest-to-junit5

Then push the new branch to your fork and create a new PR based on that branch (the PR will be identical to this one with the exception of the branch). When you've done that, we can reset your master branch.

git switch master
git fetch upstream master # fetch upstream/master without merging into local master
git reset --hard upstream/master # reset local master to upstream/master's state
git push --force origin master # force-update your fork's master to reflect upstream

That should be it. Let me know if you need any help.

@Rohitesh-Kumar-Jain
Copy link
Contributor Author

Rohitesh-Kumar-Jain commented May 24, 2021

Hi @slarse,

Thanks for giving a detailed review of the PR. I will keep each and every point in my mind from now on, and won't repeat the same mistakes again. Thanks for letting me know the importance of creating a new branch, earlier I wasn't aware why we should create a new branch, often I used to skip creating a new branch.

It's great that I am getting complete reviews of PRs, so thanks for taking so much pain in reviewing and sharing your workflow with me, it really helped. I am glad I am learning good industry practices : )

I have raised a new PR #3946, I would be glad if may receive a review on it as well.

@slarse
Copy link
Collaborator

slarse commented May 24, 2021

@Rohitesh-Kumar-Jain You're welcome! With the first few PRs you may struggle a little bit with getting everything correct, and that's fine. After you've done this a few times, it'll be second nature and you won't even think about any of the individual steps.

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