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

Simplify confusing test code and options parsing #20111

Closed
wants to merge 2 commits into from

Conversation

gregestren
Copy link
Contributor

@gregestren gregestren commented Nov 8, 2023

We can lightly refactor test code to no longer need a logical exception in Bazel's option parsing.

Supports #20107 and #20081.

@gregestren gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Nov 8, 2023
@gregestren gregestren requested review from katre and fmeum November 8, 2023 14:50
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels Nov 8, 2023
@gregestren gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Nov 8, 2023
@fmeum
Copy link
Collaborator

fmeum commented Nov 8, 2023

c0 has to be replaced in even more places, but apart from that this looks fine.

@gregestren
Copy link
Contributor Author

c0 has to be replaced in even more places, but apart from that this looks fine.

Where else?

@fmeum
Copy link
Collaborator

fmeum commented Nov 8, 2023

and other occurrences in that file.

@gregestren
Copy link
Contributor Author

and other occurrences in that file.

Ah, I didn't notice that dependency at first. Thanks.

@gregestren
Copy link
Contributor Author

Updated as requested, CI passes.

@gregestren gregestren added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 9, 2023
@sgowroji
Copy link
Member

Hi @gregestren, While merging the above PR our CI is gettin failed because of above code conflicts. https://buildkite.com/bazel/google-bazel-presubmit/builds/73609#018bb7f5-0d9d-4e61-899d-c7843bccc766

//src/test/shell/integration:starlark_configurations_test                FAILED ```

gregestren and others added 2 commits November 10, 2023 10:19
PiperOrigin-RevId: 580517309
Change-Id: I2ea3cc5b67a8a5d047972b2c85ae3a93680be612
PiperOrigin-RevId: 580517309
Change-Id: I2ea3cc5b67a8a5d047972b2c85ae3a93680be612
@gregestren
Copy link
Contributor Author

Hi @gregestren, While merging the above PR our CI is gettin failed because of above code conflicts. https://buildkite.com/bazel/google-bazel-presubmit/builds/73609#018bb7f5-0d9d-4e61-899d-c7843bccc766

//src/test/shell/integration:starlark_configurations_test                FAILED ```

Updated. Let's see how CI responds.

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants