-
Notifications
You must be signed in to change notification settings - Fork 569
[DO NOT REVIEW] Adding RetryFact and RetryTheory to all tests #5272
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?
Conversation
| { | ||
| Trace.WriteLine($"##vso[task.logissue type=warning]Test '{TestMethod.Method.Name}' failed with non-retriable exception. Skipping retries."); | ||
|
|
||
| if (lastException != null) |
Check warning
Code scanning / CodeQL
Constant condition Warning
access to local variable shouldRetry
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix this issue, remove the constant condition check if (lastException != null) at line 158. Since lastException is always non-null at this point, its inner block (assigning to aggregator and so forth) should be unconditionally executed. Replace the conditional block with a direct execution of its contents, simplifying the code and matching the intended flow without changing program semantics.
Only lines 158–161 are directly affected:
if (lastException != null)
{
aggregator.Add(lastException);
}should become simply:
aggregator.Add(lastException);No imports, definitions, or additional changes are required.
-
Copy modified line R158
| @@ -155,10 +155,7 @@ | ||
| { | ||
| Trace.WriteLine($"##vso[task.logissue type=warning]Test '{TestMethod.Method.Name}' failed with non-retriable exception. Skipping retries."); | ||
|
|
||
| if (lastException != null) | ||
| { | ||
| aggregator.Add(lastException); | ||
| } | ||
| aggregator.Add(lastException); | ||
|
|
||
| return runSummary; | ||
| } |
| } | ||
| catch (Exception ex) | ||
| { | ||
| lastException = ex; |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
lastException
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the useless assignment issue flagged on line 99 (lastException = ex;), simply remove this statement, since the assigned value to lastException is never read in this execution path (the method will always exit at the subsequent throw;). The change should be made in src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs, within the catch block that handles generic Exception ex, replacing:
lastException = ex;
throw;with just:
throw;No other code, imports, method definitions, or usage of lastException should be changed, as this assignment is not read and its removal does not affect side effects or overall method functionality.
| @@ -96,7 +96,6 @@ | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| lastException = ex; | ||
| throw; | ||
| } | ||
| } |
| if (isAssertionFailure) | ||
| { | ||
| lastException = new XunitException(fullMessage); | ||
| } | ||
| else | ||
| { | ||
| lastException = new InvalidOperationException(fullMessage); | ||
| } |
Check notice
Code scanning / CodeQL
Missed ternary opportunity Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the issue, refactor the conditional assignment to use the conditional (ternary) operator instead of an if/else statement. Specifically, in the snippet:
if (isAssertionFailure)
{
lastException = new XunitException(fullMessage);
}
else
{
lastException = new InvalidOperationException(fullMessage);
}Replace this block with a single assignment using the ternary operator:
lastException = isAssertionFailure
? new XunitException(fullMessage)
: new InvalidOperationException(fullMessage);This change should be made inside src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs, replacing lines 139–146 with the new condensed assignment. No new imports or methods are needed, as the types involved are already referenced.
-
Copy modified lines R139-R141
| @@ -136,14 +136,9 @@ | ||
| string fullMessage = failureMsg + | ||
| (stackTrace != null ? Environment.NewLine + "Stack Trace:" + Environment.NewLine + stackTrace : string.Empty); | ||
|
|
||
| if (isAssertionFailure) | ||
| { | ||
| lastException = new XunitException(fullMessage); | ||
| } | ||
| else | ||
| { | ||
| lastException = new InvalidOperationException(fullMessage); | ||
| } | ||
| lastException = isAssertionFailure | ||
| ? new XunitException(fullMessage) | ||
| : new InvalidOperationException(fullMessage); | ||
| } | ||
|
|
||
| Trace.WriteLine($"##vso[task.logdetail]RetryTheory test failed on attempt {attempt} with exception type: {lastException?.GetType().FullName ?? "null"}"); |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Describe the changes in this PR.
Related issues
Addresses [issue #].
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)