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

tools: Add check-branch to run 'make check' on all branch commits #1214

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Apr 22, 2022

What does this PR do?

This is a first pass at a script to ensure a branch has commits which individually pass make check.

Tested?

  • Manually
  • Passed linting & tests (each commit)

^ checked with this script!

Notes & Questions

This may become integrated into CI at some point, but an initial try at this turned out to be more complex than expected, so working with something we can easily run locally first.

I've not added documentation yet, since I'd like to let this get some simple testing first.

This script passes if it leaves you back at your branch; if it fails it leaves you at the failed commit, at which point you can git rebase --abort and explore it more yourself, or use the rebase interactively.

@neiljp neiljp added the area: infrastructure Project infrastructure label Apr 22, 2022
@zulipbot zulipbot added size: S [Automatic label added by zulipbot] size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Apr 22, 2022
Copy link
Collaborator

@Rohitth007 Rohitth007 left a comment

Choose a reason for hiding this comment

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

I tested this out and it seems to work fine except a few issues as stated below.
It also works with the Makefile. I tested with the below code and it works like as if you had run it directly:

check-branch:
    @ ./tools/check-branch

tools/check-branch Outdated Show resolved Hide resolved
tools/check-branch Show resolved Hide resolved
@neiljp neiljp force-pushed the 2022-04-21-check-branch-script branch from 3cccce4 to 3a39312 Compare April 23, 2022 15:51
This git command can be run manually, but is encoded here in a
reproducible format via this script, supporting use by maintainers and
new developers alike.

The branch for comparison is defined by default as from the merge-base
with upstream/main, though another comparison point can be specified as
the first parameter, eg. HEAD~2 to check the last two commits, or main
to specify the local main in case it differs from upstream/main.

The `git --no-pager log -1` ensures it is clear which commit is being
tested at each stage.

Check taken from tools/fetch-pull-request to handle situation with
uncommitted changes.

Other tools updated to exclude testing this bash script.
@neiljp neiljp force-pushed the 2022-04-21-check-branch-script branch from 3a39312 to 09accd4 Compare April 25, 2022 00:50
If current branch has no differences from main, skip check.
This would avoid checking a branch which simply makes changes and
reverts them, but this should be a rare case, and otherwise avoids
accidental extra testing of eg. main.

By default the script uses upstream/main as the merge-base; in case the
local main has differences, warn of this and require explicit
specification of the merge-base as the first parameter in order to run
the tool.
@neiljp neiljp force-pushed the 2022-04-21-check-branch-script branch from 09accd4 to 6ac90a3 Compare April 25, 2022 00:57
@neiljp neiljp merged commit 60fdbfd into zulip:main Apr 25, 2022
@neiljp neiljp added this to the Next Release milestone Apr 25, 2022
@neiljp
Copy link
Collaborator Author

neiljp commented Apr 25, 2022

@Rohitth007 Thanks for the review and testing!

Good to hear that this slightly revised version does work for you with the --no-pager change 🎉

We can consider adding to the makefile, though by default this would limit being able to pass through an alternative merge-base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: infrastructure Project infrastructure size: M [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants