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

Add tip on solving merge conflicts in functional tests #2261

Conversation

yucheng11122017
Copy link
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Add tip on solving merge conflict in functional test expected results
Tip based on comment by @tlylt

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Add tip on solving merge conflicts in functional tests


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Copy link
Contributor

@itsyme itsyme left a comment

Choose a reason for hiding this comment

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

Good work @yucheng11122017! LGTM besides the small nit! Useful tip that I think many future developers will find useful!

3. Checkout to PR branch
4. Merge local master into PR branch
- `git merge master`
5. Accept any change to conflicts in generated test files
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could include examples of files/filetypes of generated functional test files!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @itsyme, thanks for the comment! I added a tooltip explaining the location of these expected files above (expected for functional and __snapshot__ for unit test)

Copy link
Contributor

@lhw-1 lhw-1 left a comment

Choose a reason for hiding this comment

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

LGTM! Just some nits on the wording 👍

docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Show resolved Hide resolved
@yucheng11122017
Copy link
Contributor Author

Thanks for the review @lhw-1! Fixed the nits and responded to your comments :)

Copy link
Contributor

@lhw-1 lhw-1 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the nits! Unfortunately, I have more nits (sorry) 😅

docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
docs/devGuide/development/workflow.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lhw-1 lhw-1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yucheng11122017 yucheng11122017 merged commit 4a62634 into MarkBind:master Apr 4, 2023
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.

3 participants