Skip to content

Conversation

@jestradaMS
Copy link
Contributor

Description

Describe the changes in this PR.

Related issues

Addresses [issue #].

Testing

Describe how this change was tested.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@jestradaMS jestradaMS requested a review from a team as a code owner December 11, 2025 01:55
{
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

Condition is always true because of
access to local variable shouldRetry
.

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.


Suggested changeset 1
src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs b/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
--- a/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
+++ b/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
@@ -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;
                             }
EOF
@@ -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;
}
Copilot is powered by AI and may make mistakes. Always verify output.
}
catch (Exception ex)
{
lastException = ex;

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
lastException
is useless, since its value is never read.

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.


Suggested changeset 1
src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs b/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
--- a/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
+++ b/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
@@ -96,7 +96,6 @@
                     }
                     catch (Exception ex)
                     {
-                        lastException = ex;
                         throw;
                     }
                 }
EOF
@@ -96,7 +96,6 @@
}
catch (Exception ex)
{
lastException = ex;
throw;
}
}
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines +139 to +146
if (isAssertionFailure)
{
lastException = new XunitException(fullMessage);
}
else
{
lastException = new InvalidOperationException(fullMessage);
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity Note

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

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.

Suggested changeset 1
src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs b/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
--- a/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
+++ b/src/Microsoft.Health.Extensions.Xunit/RetryDelayEnumeratedTheoryTestCase.cs
@@ -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"}");
EOF
@@ -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"}");
Copilot is powered by AI and may make mistakes. Always verify output.
@jestradaMS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

2 participants