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 sandbox mode to CI testing matrix #895

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

gyfelton
Copy link
Contributor

@gyfelton gyfelton commented Aug 19, 2024

What changed:

  1. Add sandbox on/off to testing matrix, so we have 2 bazel version * VFS on/off * sandbox on off = 8 tests in total ATM
  2. Fix redefinition of AppErrorCode enum error in sandbox mode by removing the extra import in bridging header. The tests still works and can catch renaming of the enum for example. Why it was working in non sandbox mode is unknown
  3. Add sandbox config to include all settings needed to pass CI. specifically allow a small set of actions to run locally.

Why this change:
To add CI coverage to sandbox mode to ensure no regression on it, now that we have fixed issues with sandbox mode in #894

Discussion:
CI duration might be a concern, we can exclude certain combo in the test matrix if we want, it's possible with exclude keyword

@gyfelton gyfelton force-pushed the yuanfeng/add-ci-sandboxed branch 4 times, most recently from 6413d5c to 2d69588 Compare August 20, 2024 13:28
@gyfelton gyfelton changed the title Add CI sandboxed Add sandbox mode to testing matrix Aug 20, 2024
@gyfelton gyfelton marked this pull request as ready for review August 20, 2024 13:32
@gyfelton gyfelton changed the title Add sandbox mode to testing matrix Add sandbox mode to CI testing matrix Aug 20, 2024
Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

Some comments on the config. I think it's fine if there's more ci jobs just means PRs take a lil longer to merge but we'll be more confident about the change

.bazelrc Outdated Show resolved Hide resolved
.bazelrc Show resolved Hide resolved
@luispadron
Copy link
Collaborator

Updated the required check in GitHub this should be ready once CI completes

fix redefinition of enum
@gyfelton gyfelton force-pushed the yuanfeng/add-ci-sandboxed branch from 2d69588 to 8016350 Compare August 20, 2024 14:48
@gyfelton gyfelton enabled auto-merge (squash) August 20, 2024 14:48
@gyfelton gyfelton merged commit 98d49d5 into master Aug 20, 2024
13 checks passed
@gyfelton gyfelton deleted the yuanfeng/add-ci-sandboxed branch August 20, 2024 16:28
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