Skip to content
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

[CI Visibility] Fix detached head situation #6594

Merged
merged 6 commits into from
Feb 6, 2025

Conversation

daniel-romano-DD
Copy link
Contributor

@daniel-romano-DD daniel-romano-DD commented Jan 27, 2025

Summary of changes

In a detached head situation, create a synthetic branch name like auto:git-detached-head.

Reason for change

An empty branch name (as when a detached head happens) leads to many of the TestVis features to be disabled.

Implementation details

Test coverage

Other details

@github-actions github-actions bot added the area:tests unit tests, integration tests label Jan 27, 2025
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Jan 27, 2025

Datadog Report

Branch report: dani/testvis/impactedtests_alpine_bug
Commit report: 13ee2b3
Test service: dd-trace-dotnet

✅ 0 Failed, 253088 Passed, 2442 Skipped, 18h 43m 7.49s Total Time

@andrewlock
Copy link
Member

andrewlock commented Jan 27, 2025

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6594) - mean (69ms)  : 66, 72
     .   : milestone, 69,
    master - mean (69ms)  : 66, 72
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6594) - mean (991ms)  : 960, 1022
     .   : milestone, 991,
    master - mean (985ms)  : 957, 1014
     .   : milestone, 985,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6594) - mean (102ms)  : 100, 104
     .   : milestone, 102,
    master - mean (102ms)  : 99, 105
     .   : milestone, 102,

    section CallTarget+Inlining+NGEN
    This PR (6594) - mean (666ms)  : 648, 685
     .   : milestone, 666,
    master - mean (673ms)  : 660, 686
     .   : milestone, 673,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6594) - mean (90ms)  : 88, 91
     .   : milestone, 90,
    master - mean (89ms)  : 87, 91
     .   : milestone, 89,

    section CallTarget+Inlining+NGEN
    This PR (6594) - mean (626ms)  : 610, 643
     .   : milestone, 626,
    master - mean (633ms)  : 617, 649
     .   : milestone, 633,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6594) - mean (191ms)  : 187, 194
     .   : milestone, 191,
    master - mean (191ms)  : 186, 195
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (6594) - mean (1,096ms)  : 1065, 1126
     .   : milestone, 1096,
    master - mean (1,102ms)  : 1069, 1135
     .   : milestone, 1102,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6594) - mean (270ms)  : 267, 274
     .   : milestone, 270,
    master - mean (270ms)  : 264, 277
     .   : milestone, 270,

    section CallTarget+Inlining+NGEN
    This PR (6594) - mean (858ms)  : 824, 892
     .   : milestone, 858,
    master - mean (864ms)  : 831, 896
     .   : milestone, 864,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6594) - mean (261ms)  : 257, 264
     .   : milestone, 261,
    master - mean (261ms)  : 257, 264
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (6594) - mean (840ms)  : 813, 867
     .   : milestone, 840,
    master - mean (843ms)  : 812, 875
     .   : milestone, 843,

Loading

@andrewlock
Copy link
Member

andrewlock commented Jan 27, 2025

Benchmarks Report for tracer 🐌

Benchmarks for #6594 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.283
  • 2 benchmarks are slower, with geometric mean 1.134
  • All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 8.33μs 47.9ns 377ns 0.0162 0.00808 0 5.61 KB
master StartStopWithChild netcoreapp3.1 10.5μs 56.3ns 333ns 0.0162 0.00542 0 5.8 KB
master StartStopWithChild net472 16.5μs 56ns 217ns 1.05 0.311 0.106 6.22 KB
#6594 StartStopWithChild net6.0 7.99μs 44.2ns 286ns 0.0155 0.00777 0 5.61 KB
#6594 StartStopWithChild netcoreapp3.1 10.4μs 53.5ns 267ns 0.0211 0.0106 0 5.8 KB
#6594 StartStopWithChild net472 16.4μs 53.2ns 206ns 1.05 0.324 0.105 6.21 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 498μs 667ns 2.49μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 646μs 500ns 1.87μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 856μs 852ns 3.3μs 0.425 0 0 3.3 KB
#6594 WriteAndFlushEnrichedTraces net6.0 487μs 212ns 820ns 0 0 0 2.7 KB
#6594 WriteAndFlushEnrichedTraces netcoreapp3.1 644μs 304ns 1.18μs 0 0 0 2.7 KB
#6594 WriteAndFlushEnrichedTraces net472 859μs 476ns 1.84μs 0.428 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 126μs 482ns 1.87μs 0.193 0 0 14.47 KB
master SendRequest netcoreapp3.1 144μs 355ns 1.37μs 0.214 0 0 17.27 KB
master SendRequest net472 0.0126ns 0.00446ns 0.0173ns 0 0 0 0 b
#6594 SendRequest net6.0 127μs 571ns 2.21μs 0.19 0 0 14.47 KB
#6594 SendRequest netcoreapp3.1 147μs 353ns 1.37μs 0.22 0 0 17.27 KB
#6594 SendRequest net472 0.00995ns 0.00265ns 0.0103ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 550μs 2.72μs 11.2μs 0.543 0 0 41.66 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 653μs 2.09μs 8.35μs 0.327 0 0 41.8 KB
master WriteAndFlushEnrichedTraces net472 851μs 3.76μs 14.6μs 8.33 2.5 0.417 53.3 KB
#6594 WriteAndFlushEnrichedTraces net6.0 570μs 2.78μs 11.8μs 0.563 0 0 41.53 KB
#6594 WriteAndFlushEnrichedTraces netcoreapp3.1 654μs 3.31μs 15.5μs 0.327 0 0 41.79 KB
#6594 WriteAndFlushEnrichedTraces net472 847μs 3.85μs 15.4μs 8.63 2.47 0.411 53.31 KB
Benchmarks.Trace.DbCommandBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6594

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net6.0 1.119 1,224.18 1,370.20

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.22μs 0.531ns 1.99ns 0.0142 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.82μs 4.48ns 17.3ns 0.0141 0 0 1.02 KB
master ExecuteNonQuery net472 2.06μs 1.49ns 5.57ns 0.156 0.00104 0 987 B
#6594 ExecuteNonQuery net6.0 1.37μs 0.772ns 2.99ns 0.0144 0 0 1.02 KB
#6594 ExecuteNonQuery netcoreapp3.1 1.78μs 1.36ns 5.28ns 0.0135 0 0 1.02 KB
#6594 ExecuteNonQuery net472 2.04μs 1.8ns 6.96ns 0.157 0.00102 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.18μs 0.549ns 2.05ns 0.0136 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.53μs 0.6ns 2.16ns 0.013 0 0 976 B
master CallElasticsearch net472 2.56μs 2.07ns 8.03ns 0.157 0 0 995 B
master CallElasticsearchAsync net6.0 1.26μs 0.759ns 2.84ns 0.0133 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.52μs 1.22ns 4.58ns 0.0136 0 0 1.02 KB
master CallElasticsearchAsync net472 2.64μs 1.41ns 5.47ns 0.166 0 0 1.05 KB
#6594 CallElasticsearch net6.0 1.27μs 0.609ns 2.36ns 0.0134 0 0 976 B
#6594 CallElasticsearch netcoreapp3.1 1.45μs 0.552ns 1.99ns 0.0132 0 0 976 B
#6594 CallElasticsearch net472 2.65μs 1.05ns 4.08ns 0.158 0 0 995 B
#6594 CallElasticsearchAsync net6.0 1.33μs 0.766ns 2.97ns 0.0133 0 0 952 B
#6594 CallElasticsearchAsync netcoreapp3.1 1.55μs 0.674ns 2.52ns 0.0132 0 0 1.02 KB
#6594 CallElasticsearchAsync net472 2.75μs 1.22ns 4.57ns 0.167 0 0 1.05 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net6.0 1.29μs 1.39ns 5.37ns 0.0135 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.58μs 0.814ns 3.15ns 0.0126 0 0 952 B
master ExecuteAsync net472 1.85μs 0.392ns 1.46ns 0.145 0 0 915 B
#6594 ExecuteAsync net6.0 1.35μs 0.461ns 1.72ns 0.0136 0 0 952 B
#6594 ExecuteAsync netcoreapp3.1 1.63μs 1.65ns 6.41ns 0.0129 0 0 952 B
#6594 ExecuteAsync net472 1.82μs 0.386ns 1.45ns 0.145 0 0 915 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.56μs 1.94ns 7.51ns 0.0315 0 0 2.31 KB
master SendAsync netcoreapp3.1 5.12μs 2.3ns 8.91ns 0.0383 0 0 2.85 KB
master SendAsync net472 7.43μs 3.1ns 12ns 0.493 0 0 3.12 KB
#6594 SendAsync net6.0 4.39μs 2.54ns 9.5ns 0.0328 0 0 2.31 KB
#6594 SendAsync netcoreapp3.1 5.38μs 3.83ns 14.8ns 0.0375 0 0 2.85 KB
#6594 SendAsync net472 7.46μs 1.05ns 3.78ns 0.495 0 0 3.12 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.46μs 0.628ns 2.43ns 0.0234 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.1μs 1.57ns 6.08ns 0.0221 0 0 1.64 KB
master EnrichedLog net472 2.52μs 0.601ns 2.17ns 0.249 0 0 1.57 KB
#6594 EnrichedLog net6.0 1.53μs 1.37ns 5.11ns 0.0231 0 0 1.64 KB
#6594 EnrichedLog netcoreapp3.1 2.12μs 1.37ns 4.94ns 0.0215 0 0 1.64 KB
#6594 EnrichedLog net472 2.74μs 0.905ns 3.51ns 0.25 0 0 1.57 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 112μs 117ns 453ns 0.056 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 117μs 184ns 715ns 0.0579 0 0 4.28 KB
master EnrichedLog net472 149μs 58.5ns 211ns 0.669 0.223 0 4.46 KB
#6594 EnrichedLog net6.0 115μs 210ns 815ns 0.0561 0 0 4.28 KB
#6594 EnrichedLog netcoreapp3.1 115μs 121ns 467ns 0.057 0 0 4.28 KB
#6594 EnrichedLog net472 151μs 190ns 710ns 0.676 0.225 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.03μs 1.06ns 3.82ns 0.0302 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.28μs 1.7ns 6.35ns 0.0299 0 0 2.2 KB
master EnrichedLog net472 4.79μs 1.35ns 5.21ns 0.32 0 0 2.02 KB
#6594 EnrichedLog net6.0 3.07μs 1.43ns 5.52ns 0.0307 0 0 2.2 KB
#6594 EnrichedLog netcoreapp3.1 4.33μs 2.79ns 10.8ns 0.0283 0 0 2.2 KB
#6594 EnrichedLog net472 4.86μs 1.67ns 6.48ns 0.321 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.38μs 0.81ns 3.14ns 0.0159 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.79μs 0.798ns 3.09ns 0.0152 0 0 1.14 KB
master SendReceive net472 2μs 0.715ns 2.77ns 0.183 0 0 1.16 KB
#6594 SendReceive net6.0 1.4μs 0.7ns 2.71ns 0.0161 0 0 1.14 KB
#6594 SendReceive netcoreapp3.1 1.79μs 1.01ns 3.93ns 0.0149 0 0 1.14 KB
#6594 SendReceive net472 2.17μs 0.86ns 3.33ns 0.183 0 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.9μs 0.839ns 3.14ns 0.0218 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.84μs 1.61ns 6.22ns 0.0212 0 0 1.65 KB
master EnrichedLog net472 4.42μs 2.41ns 9.35ns 0.322 0 0 2.04 KB
#6594 EnrichedLog net6.0 2.93μs 1.01ns 3.9ns 0.0219 0 0 1.6 KB
#6594 EnrichedLog netcoreapp3.1 3.81μs 1.7ns 6.36ns 0.021 0 0 1.65 KB
#6594 EnrichedLog net472 4.45μs 2.24ns 8.37ns 0.324 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6594

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 1.148 494.69 567.87

Faster 🎉 in #6594

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1 1.283 849.07 661.73

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 398ns 0.244ns 0.915ns 0.00818 0 0 576 B
master StartFinishSpan netcoreapp3.1 575ns 0.465ns 1.8ns 0.00778 0 0 576 B
master StartFinishSpan net472 636ns 0.537ns 2.08ns 0.0918 0 0 578 B
master StartFinishScope net6.0 495ns 0.285ns 1.07ns 0.00984 0 0 696 B
master StartFinishScope netcoreapp3.1 848ns 1.45ns 5.6ns 0.00934 0 0 696 B
master StartFinishScope net472 912ns 0.943ns 3.65ns 0.104 0 0 658 B
#6594 StartFinishSpan net6.0 408ns 0.259ns 1ns 0.00814 0 0 576 B
#6594 StartFinishSpan netcoreapp3.1 621ns 0.287ns 1.11ns 0.00783 0 0 576 B
#6594 StartFinishSpan net472 578ns 0.191ns 0.739ns 0.0916 0 0 578 B
#6594 StartFinishScope net6.0 568ns 0.277ns 1.07ns 0.00982 0 0 696 B
#6594 StartFinishScope netcoreapp3.1 662ns 0.163ns 0.608ns 0.00932 0 0 696 B
#6594 StartFinishScope net472 837ns 0.233ns 0.807ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 696ns 0.768ns 2.97ns 0.0101 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 944ns 0.888ns 3.44ns 0.00941 0 0 696 B
master RunOnMethodBegin net472 1.14μs 0.387ns 1.5ns 0.104 0 0 658 B
#6594 RunOnMethodBegin net6.0 664ns 0.387ns 1.5ns 0.00987 0 0 696 B
#6594 RunOnMethodBegin netcoreapp3.1 1.01μs 0.29ns 1.12ns 0.00939 0 0 696 B
#6594 RunOnMethodBegin net472 1.13μs 0.28ns 1.05ns 0.104 0 0 658 B

@daniel-romano-DD daniel-romano-DD force-pushed the dani/testvis/impactedtests_alpine_bug branch from b9ec16e to a413669 Compare January 28, 2025 23:04
@daniel-romano-DD daniel-romano-DD changed the title Enable debug traces [TestVis] Fix detached head situation Jan 29, 2025
@daniel-romano-DD daniel-romano-DD force-pushed the dani/testvis/impactedtests_alpine_bug branch from a413669 to 27810e1 Compare February 3, 2025 10:43
@daniel-romano-DD daniel-romano-DD changed the title [TestVis] Fix detached head situation [Ci-Visibility] Fix detached head situation Feb 3, 2025
@daniel-romano-DD daniel-romano-DD changed the title [Ci-Visibility] Fix detached head situation [CI Visibility] Fix detached head situation Feb 3, 2025
@daniel-romano-DD daniel-romano-DD marked this pull request as ready for review February 3, 2025 15:33
@daniel-romano-DD daniel-romano-DD requested review from a team as code owners February 3, 2025 15:33
@daniel-romano-DD daniel-romano-DD force-pushed the dani/testvis/impactedtests_alpine_bug branch from 27810e1 to 13ee2b3 Compare February 5, 2025 22:00
Copy link
Member

@andrewlock andrewlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor questions and suggestions

Comment on lines 91 to +92
_commitSha = GetCommitSha();
_branchName = GetBranchName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but the fact that you have to rearrange these seems like a great reason that these methods should be static, and should have their dependencies passed in as parameters. It would make this rearranging rquirement more obvious 🙂

Not for this PR, just flagging it 😄

@@ -1269,7 +1270,15 @@ private string GetBranchName()
}

var gitOutput = GitCommandHelper.RunGitCommand(_workingDirectory, "branch --show-current", MetricTags.CIVisibilityCommands.GetBranch);
return gitOutput?.Output.Replace("\n", string.Empty) ?? string.Empty;
var res = gitOutput?.Output.Replace("\n", string.Empty) ?? string.Empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably handle Windows too? 🤔

Suggested change
var res = gitOutput?.Output.Replace("\n", string.Empty) ?? string.Empty;
var res = gitOutput?.Output
.Replace("\r\n", string.Empty)
.Replace("\n", string.Empty)
?? string.Empty;

if (string.IsNullOrEmpty(res))
{
Log.Warning("ITR: empty branch indicates a detached head at commit {Commit}", _commitSha);
res = $"auto:git-detached-head";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is standardised across languages now 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are pioneers, but yep :)

Comment on lines +245 to +251
if (output.ExitCode == 0)
{
// Retrieve branch name
branch = output.Output.Trim();
}

if (output.ExitCode == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can combine these checks?

Suggested change
if (output.ExitCode == 0)
{
// Retrieve branch name
branch = output.Output.Trim();
}
if (output.ExitCode == 0)
if (output.ExitCode == 0)
{
// Retrieve branch name
branch = output.Output.Trim();

tbh, I think this whole method would be easier to follow with early-returns. It's a bit confusing given we have various nested if (many of which are unnecessary).

Comment on lines +254 to +255
output = RunGitCommand("rev-parse --verify HEAD");
if (output.ExitCode == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails, you now no-longer run the RunGitCommand("rev-parse --show-toplevel"); block below. Are you sure that's what you want to do?

@daniel-romano-DD daniel-romano-DD merged commit 8fb40aa into master Feb 6, 2025
138 of 140 checks passed
@daniel-romano-DD daniel-romano-DD deleted the dani/testvis/impactedtests_alpine_bug branch February 6, 2025 14:32
@github-actions github-actions bot added this to the vNext-v3 milestone Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ci-visibility area:tests unit tests, integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants