-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Expand GitHub Actions test matrix #4371
Comments
Maybe we can use this issue to spec out the full matrix of configurations we want to test. I'll do some digging and come back with what I think matches what we had or were trying to do months ago. |
I found these resources:
I think the matrix in the README for #3851 is the one we want to implement. Before I start on that, I'm going to work on making sure our self-hosted runners are ready. |
As mentioned in #4454 (comment), we should add tests across the Docker images that we share (for now in https://github.com/thejohnfreeman/rippled-docker/). |
This change makes progress on the plan in #4371. It does not replicate the full [matrix] implemented in #3851, but it does replicate the 1.ii section of the Linux matrix. It leverages "heavy" self-hosted runners, and demonstrates a repeatable pattern for future matrices. [matrix]: https://github.com/XRPLF/rippled/blob/d794a0f3f161bb30c74881172fc38f763d7d46e8/.github/README.md#continuous-integration
|
Add tests across compiler versions too. |
@thejohnfreeman at your convenience, could you comment here with an update on this issue? Any blockers? |
When running unit tests with PRs, the new runners appear to try and build only release mode code. That's not bad, but it misses out on opportunities to capture both compile time issues (e.g. compilation errors in debug-only code) and runtime issues (e.g. potential
assert
failures that are intentionally swallowed up in release builds, or debug-only code both in libraries and our own code).Fix: unit tests should be executed in both debug and release configurations. I guess this is probably an issue for @legleux.
The text was updated successfully, but these errors were encountered: