Add global configuration#305
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Warning: included a breaking change by changing the named ctor parameters to instead be properties. Will need a major version bump when released. That's fine though as v3 is not out yet & for older ones a major bump to align stuff is ok. The attribute property syntax makes this fine for end-users, but it switches from camelCase to TitleCase. This was done as the project level defaults makes ctor parameters a little painful, needing a magic value as the default for them to work, as we can't use the default values (3, 0) any more, since they should not override config. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e2249d15b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public RetryFactAttribute( | ||
| int maxRetries = DEFAULT_MAX_RETRIES, | ||
| int delayBetweenRetriesMs = DEFAULT_DELAY_BETWEEN_RETRIES_MS, | ||
| int maxRetries, | ||
| int delayBetweenRetriesMs, | ||
| params Type[] skipOnExceptions) |
There was a problem hiding this comment.
Changing this constructor to require both positional integers breaks existing source that used the previously supported optional-argument form like [RetryFact(delayBetweenRetriesMs: 100)] (and the analogous RetryTheory form), which now fails to compile after upgrading. This is a user-facing source compatibility regression for callers that only wanted to override delay, so an overload or compatible signature should be kept to avoid breaking existing test projects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not possible because the fallbacks are no longer compile-time constants. They could come from the global config instead.
This new way to do this is to set the public property, e.g. [RetryFact(DelayBetweenRetriesMs = 100)]. That works with the global config or the constants if there's no global config, and it just as easy to use.
This is a (minor) breaking change though & will need a mention in the release notes.
JoshKeegan
force-pushed
the
global-config
branch
2 times, most recently
from
ab89d32 to
8acac23
Compare
Introduces project-wide defaults, with a new file `xretry.json` supporting maxRetries and delayBetweenRetriesMs for all test runners. Implements #77. BDD test runners (SpecFlow, Reqnroll and Reqnroll on xUnit.v3) also have retryUntaggedScenarios. This becomes a way of making all scenarios retried by default, with no test changes required. This is only possible for the BDD tests, since the xUnit tests require use of different attributes (RetryFact/RetryTheory rather than Fact/Theory). Implements #78. It felt logical to add retryUntaggedScenarios at the same time as global config, not as a separate change because once global config is introduced and you can configure maxRetries for the entire project, I expect people will try and use it without tagging individual features or scenarios. Having a property of the global config for this behaviour makes it clear, with examples in docs, and anyone who wants this can easily enable it. Defaults are unchanged, i.e.: max retries = 3 delay between attempts = 0 retry untagged scenarios = false That keeps this a non-breaking change, and means when you add xRetry to any project it won't affect tests without you either opting-in on a per test/feature/scenario basis, or adding global config. A schema should be added to the xretry.json file, including examples that people will copy so they get schema validation in their IDE. Will add this once it's in master so the URL isn't to a branch.
JoshKeegan
force-pushed
the
global-config
branch
from
c1f448f to
acf879f
Compare
1 participant
Introduces project-wide defaults, with a new file
xretry.jsonsupportingmaxRetriesanddelayBetweenRetriesMsfor all test runners. Implements #77.BDD test runners (SpecFlow, Reqnroll and Reqnroll on xUnit.v3) also have
retryUntaggedScenarios. This becomes a way of making all scenarios retried by default, with no test changes required. This is only possible for the BDD tests, since the xUnit tests require use of different attributes (RetryFact/RetryTheoryrather thanFact/Theory). Implements #78.It felt logical to add
retryUntaggedScenariosat the same time as global config, not as a separate change because once global config is introduced and you can configuremaxRetriesfor the entire project, I expect people will try and use it without tagging individual features or scenarios. Having a property of the global config for this behaviour makes it clear, with examples in docs, and anyone who wants this can easily enable it.Defaults are unchanged, i.e.:
That keeps this a non-breaking change, and means when you add xRetry to any project it won't affect tests without you either opting-in on a per test/feature/scenario basis, or adding global config.
A schema should be added to the xretry.json file, including examples that people will copy so they get schema validation in their IDE. Will add this once it's in master so the URL isn't to a branch.