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 Github action to run Bazel tests for every PR #159

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

jasongraffius
Copy link
Collaborator

This will ensure that all pull requests must pass all Bazel tests in the repository. Merging the pull request will be blocked until the tests are passing, and will re-run each time the pull request is synchronized or updated.

This will ensure that all pull requests must pass all Bazel tests in the
repository. Merging the pull request will be blocked until the tests are
passing, and will re-run each time the pull request is synchronized or
updated.
@jasongraffius
Copy link
Collaborator Author

jasongraffius commented Jul 20, 2024

For an example of what this looks like, see the check for "Run Bazel tests / run-bazel-tests (pull_request)" under "Show all checks" below. The run is configured to use Bazel's caching so subsequent runs of small PRs should be fairly quick.

Copy link
Collaborator

@EricRahm EricRahm left a comment

Choose a reason for hiding this comment

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

This is a big improvement!

I'm fine landing as-is. I can imagine wanting to expand things as follow ups. A few off the top of my head:

  • auto-formatting
  • pre-submit hook
  • type checking (mypy or something like that)

There's also semi-related #136 where our minimum python version, 3.8, fails to run. Since this action isn't failing I'm assuming whatever version is in stock ubuntu-latest is fine, but we might consider either specifying a version to install or landing something like #137 where we configure bazel to use a hermetic python install.

It also might make sense to expand to run tests against all supported python versions (and maybe a few c++ compilers).

We should just file some issues and follow-up there, but I thought it'd be helpful to think about while you're doing this.

@jasongraffius
Copy link
Collaborator Author

jasongraffius commented Jul 22, 2024

This is a big improvement!

I'm fine landing as-is. I can imagine wanting to expand things as follow ups. A few off the top of my head:

  • auto-formatting

Good point! I didn't think about auto-formatting. Agreed, I think this would be an improvement as well, but probably want to resolve #76 first so that there's a standard formatting.

  • pre-submit hook

Haha, you've read my mind with this one, I was putting one together but had other obligations over the weekend, I'll probably have something ready for this soon.

  • type checking (mypy or something like that)

Same thing! Read my mind, I was looking into this too! My typecheck failed locally when I tried this, but I was looking into any configuration I need to do first. If we get a passing typecheck it will be straightforward to add this.

There's also semi-related #136 where our minimum python version, 3.8, fails to run. Since this action isn't failing I'm assuming whatever version is in stock ubuntu-latest is fine, but we might consider either specifying a version to install or landing something like #137 where we configure bazel to use a hermetic python install.

It also might make sense to expand to run tests against all supported python versions (and maybe a few c++ compilers).

We should just file some issues and follow-up there, but I thought it'd be helpful to think about while you're doing this.

Well now that's three in a row, haha. I was considering adding an action that runs unit tests in both an "earliest-supported" and a "latest-tested" python versions, to guard both against breaking support for old versions and to catch any issues from upcoming versions (though the latter should be much, much more rare considering the intended API guarantees of python 3). That said, I think we should also set the hermetic python version for Bazel since we want it to always use a supported version.

@jasongraffius
Copy link
Collaborator Author

Added issues #160, #161, and #162 to cover the cases discussed here.

@jasongraffius jasongraffius merged commit 084992d into google:master Jul 22, 2024
2 checks passed
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