-
Notifications
You must be signed in to change notification settings - Fork 120
New constructor for envvar-based condition trait creation #1265
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
base: main
Are you sure you want to change the base?
New constructor for envvar-based condition trait creation #1265
Conversation
The new condition trait constructor makes it easy to skip/unskip tests based on the *presence* of a specific environment variable. It does not attempt to inspect the value to determine if it is "truthy" or not. Need to resolve the issue of thread safety with the envvar unit tests added here before merging. Some alternative options/extensions to consider: * **Allow specifying multiple environment variables?** Could use a variadic or something here to allow that, although maybe that is an exceedingly rare case. The disadvantage would be that it would be a little ambiguous if you need to match on "all" or "any" env variable with this approach. The current implementation has the advantage of being quite simple and probably meeting the majority of use cases. * **Check for truthy/falsy values for matching env vars?** For example, 0/false is considered "falsy", everything else considered "truthy". Then enabled means the env var is present with a truthy value, and disabled means the env var is missing or present with a falsy value. The main disadvantage here is that this logic would be more complicated and perhaps more surprising to users of the condition trait.
This is a lot of code when a developer can already write: .enabled(if: getenv("ABC123") != nil) What's the justification for this? |
@@ -55,4 +67,31 @@ struct ConditionTraitTests { | |||
result = try await enabledFalse.evaluate() | |||
#expect(!result) | |||
} | |||
|
|||
// TODO: What do we wanna do about envvar thread safety? This won't be safe to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See our existing tests that modify environment variables.
@@ -37,15 +37,27 @@ struct ConditionTraitTests { | |||
.disabled(if: false) | |||
) | |||
func disabledTraitIf() throws {} | |||
|
|||
|
|||
@Test( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be marked .hidden
. We have infrastructure for testing fixture tests and verifying they are (or are not) run.
/// environment variable. | ||
@_spi(Experimental) | ||
public static func enabled( | ||
ifEnvironmentPresent environmentName: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single environment variable is not "an environment" so we need to bikeshed this parameter label and name.
.disabled(ifEnvironmentPresent: "ABC123") Is slightly more characters to write than: .disabled(if: getenv("ABC123") != nil) And more-more after we bikeshed the label because it's probably going to say 😶 |
Agree this is better handled outside Swift Testing. In part because clients may also have different means of reading the environment. For example in Swift Build we access the environment solely through task locals in order to allow mocking it in tests. |
The new condition trait constructor makes it easy to skip/unskip tests based on the presence of a specific environment variable.
Motivation:
A common* use case for environment variables is to enable or skip specific tests. More specifically, imagine you have a test plan that uses environment variables to adjust other test parameters, like I dunno, Address Sanitizer or something. Then some tests you have might not be compatible with that, so needs to be skipped with the ASan configuration. In that case, you could do:
* Admittedly citation needed
Modifications:
Add new constructors that allow create condition traits that depend on environment variable contents. It does not attempt to inspect the value to determine if it is "truthy" or not.
Checklist:
Need to resolve the issue of thread safety with the envvar unit tests added here before merging.
Some alternative options/extensions to consider:
Allow specifying multiple environment variables? Could use a variadic or something here to allow that, although maybe that is an exceedingly rare case.
The disadvantage would be that it would be a little ambiguous if you need to match on "all" or "any" env variable with this approach. The current implementation has the advantage of being quite simple and probably meeting the majority of use cases.
Check for truthy/falsy values for matching env vars? For example, 0/false is considered "falsy", everything else considered "truthy". Then enabled means the env var is present with a truthy value, and disabled means the env var is missing or present with a falsy value.
The main disadvantage here is that this logic would be more complicated and perhaps more surprising to users of the condition trait.