Skip to content

add WORKERD_ENABLE_ALL_AUTOGATES env variable #4124

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented May 9, 2025

Ref: #4089

Adds an environment variable called "WORKERD_ENABLE_ALL_AUTOGATES" which enables all autogates regardless of what's set in the config. This is beneficial for testing.

@anonrig anonrig requested review from kentonv, npaun and danlapid May 9, 2025 23:36
@anonrig anonrig requested review from a team as code owners May 9, 2025 23:36
@anonrig anonrig requested a review from jasnell May 9, 2025 23:41
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch from e485f57 to 607163f Compare May 9, 2025 23:42
@jasnell
Copy link
Collaborator

jasnell commented May 10, 2025

I'm not entirely sure this is the right approach but could be convinced. The key reason is that this potentially misses the various combinations of some-autogates-on-some-autogates-off that could expose bugs and might be missed by this... For instance, the code might work if both autogates A and B are off or both are on, but does it work if only one of the two are on and the other is off? What I think might be better is a bazel configuration that automatically sets up variations of each test for each combination of autogates. That is, if we have three active autogates, then we would have eight test permutations covering each of the various combinations.

@danlapid
Copy link
Collaborator

I'm not entirely sure this is the right approach but could be convinced. The key reason is that this potentially misses the various combinations of some-autogates-on-some-autogates-off that could expose bugs and might be missed by this... For instance, the code might work if both autogates A and B are off or both are on, but does it work if only one of the two are on and the other is off? What I think might be better is a bazel configuration that automatically sets up variations of each test for each combination of autogates. That is, if we have three active autogates, then we would have eight test permutations covering each of the various combinations.

This PR mimics what we have internally, we lack the same detail with our internal tests.
It's a balance between coverage and test times.
It is reasonable to want better coverage but I think the request is out of scope for this PR.
This mimics correctly what we have internally and I think it is reasonable.

@kentonv
Copy link
Member

kentonv commented May 10, 2025

Should we consider using an environment variable instead, so that we can also run KJ tests with all-autogates?

That is, if we have three active autogates, then we would have eight test permutations covering each of the various combinations.

Unfortunately this doesn't work because it doesn't scale. Even 8x is already probably too much, but in the internal codebase we have 21 autogates defined currently, in addition to the 3 in workerd, that's 24 autogates, so we'd be running each test 16 million times. Granted many of them should probably be cleaned up but still...

I think running one with all off and once with all on is a reasonable compromise. Autogates rarely interact with each other in practice.

@anonrig
Copy link
Member Author

anonrig commented May 10, 2025

Should we consider using an environment variable instead, so that we can also run KJ tests with all-autogates?

I don't have any strong opinion. I'm fine with replacing the cli flag with an environment variable.

@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch 2 times, most recently from 55d6eb2 to f6d575d Compare May 12, 2025 12:57
@anonrig anonrig changed the title introduce --all-autogates cli flag add WORKERD_ENABLE_ALL_AUTOGATES env variable May 12, 2025
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch from f6d575d to 868953d Compare May 12, 2025 12:59
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch 2 times, most recently from 5aecb8b to 9827401 Compare May 12, 2025 16:29
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch 4 times, most recently from 6e3cd39 to 755e23a Compare May 12, 2025 21:04
@anonrig anonrig requested review from kentonv and danlapid May 12, 2025 21:04
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch from 755e23a to f46e788 Compare May 12, 2025 22:48
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch from f46e788 to f72b146 Compare May 12, 2025 22:50
@anonrig anonrig force-pushed the yagiz/add-all-autogates-cli-flag branch from f72b146 to 6a101f1 Compare May 12, 2025 22:51
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.

wd_test should create a variant of each test with all autogates enabled
7 participants