Skip to content

PR Pipeline - Part 1#4308

Closed
benrr101 wants to merge 44 commits into
mainfrom
dev/russellben/prci-buildstage
Closed

PR Pipeline - Part 1#4308
benrr101 wants to merge 44 commits into
mainfrom
dev/russellben/prci-buildstage

Conversation

@benrr101
Copy link
Copy Markdown
Contributor

@benrr101 benrr101 commented May 26, 2026

Description

As the goal of this sprint, we want to create a new PR pipeline that simplifies all the complexities of the old one, reduces the test coverage to tests that can run simply and quickly, and completes quickly. This PR introduces the basics of that with a few more changes to come.

  • Test execution is gated by a build/pack stage - if PR doesn't build/pack successfully, test run fails fast. ~3min
  • Test matrix is split into the following platforms as stages in parallel:
    • [Windows/Linux]
    • [net462/net8.0/net9.0/net10.0]
  • Test projects that run as jobs within each stage in parallel:
    • Abstractions ~2min
    • Azure ~2min
    • SqlClient Funtional ~3min
    • SqlClient Manual - Local ~35min
    • SqlClient Unit ~6min

This puts the total run time at ~40min in its current state. Compared to the current pipeline which has full runtimes of anywhere from 1.5hrs to 3hrs, this is a massive improvement.

Noteworthy notes have been added inline.

Remaining work

  • SqlClient Manual tests against Azure SQL
  • Code coverage collection and publishing
  • Operationalize the pipeline via triggers

Testing

Copilot AI review requested due to automatic review settings May 26, 2026 22:17
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board May 26, 2026
@benrr101 benrr101 added the Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems. label May 26, 2026
@benrr101 benrr101 added this to the 7.1.0-preview2 milestone May 26, 2026

This comment was marked as outdated.

@edwardneal
Copy link
Copy Markdown
Contributor

Is there any chance we could also fix both the PR and the main pipeline to respect the [PlatformSpecific(TestPlatforms.Windows)] pipeline? This is currently in use in DistributedTransactionTestWindows but the test criteria don't exclude it based on the CI leg's OS platform.

[PlatformSpecific] is defined in dotnet/arcade here, and its discoverer sets the category criteria here.

Copilot AI review requested due to automatic review settings May 27, 2026 18:03

This comment was marked as outdated.

@benrr101
Copy link
Copy Markdown
Contributor Author

@edwardneal - you mean the PlatformSpecific attributes aren't being respected?

@benrr101 benrr101 force-pushed the dev/russellben/prci-buildstage branch from cda511d to 3afd216 Compare May 28, 2026 18:24
@@ -4,11 +4,9 @@
# See the LICENSE file in the project root for more information. #
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These variables were always meant to be common to all pipelines.

- name: BUILD_OUTPUT
value: $(REPO_ROOT)/artifacts

# Directory where downloaded pipeline artifacts (NuGet packages from earlier
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These were moved to the onebranch-variables file

Comment on lines +1 to +12
#################################################################################
# Licensed to the .NET Foundation under one or more agreements. #
# The .NET Foundation licenses this file to you under the MIT license. #
# See the LICENSE file in the project root for more information. #
#################################################################################

name: $(DayOfYear)$(Rev:rr)

variables:
- template: /eng/pipelines/common/variables/common-variables.yml@self
- template: /eng/pipelines/pr/variables/pr-variables.yml@self

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will be covered in another PR to operationalize the pipeline.

Comment on lines +43 to +45
pool:
vmImage: 'ubuntu-latest'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This isn't actually a problem - the pack target will fail if the windows binaries aren't built, and I can confirm that the job is passing as well as the output contains a full build.

Comment on lines +37 to +40
"WorkloadIdentityFederationServiceConnectionId": "",
"KerberosDomainPassword": "",
"KerberosDomainuser": "",
"IsManagedInstance": false
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fixed in #4297, but I'll also address it here.

int numberOfRows = 10;

// Insert a bunch of rows in to the table.
// Insert a bunch of rows in to the table.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All the following changes were auto "trim whitespace on save" behavior...

IsSqlVectorSupported &&
CheckVectorFloat16Supported();

public static bool IsDebugBuild
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This enables us to skip tests if build configuration is DEBUG (or not). This is helpful because skipping will show that the test was skipped in test results, while #if DEBUG will omit it from complication and pretend the test never existed.

public static void ConnectToSQLWithInstanceNameTest()
{
SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionString);
DataTestUtility.ParseDataSource(builder.DataSource, out string hostname, out _, out _);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change was to address an issue where the tcp connection string parsing was brittle and couldn't handle hostnames with instance names or ports properly.

{
// Misc constants
private const int CALLBACK_TIMEOUT = 5000; // milliseconds
private const int CALLBACK_TIMEOUT = 20000; // milliseconds
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was an initial attempt to improve this test on linux, but it didn't make a difference. Test has just been marked flaky.

public string TCPConnectionStringHGSVBS = null;
public string TCPConnectionStringNoneVBS = null;
public string TCPConnectionStringAASSGX = null;
public bool EnclaveEnabled = false;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These changes are also being made in #4297

@edwardneal
Copy link
Copy Markdown
Contributor

That's correct @benrr101 - [PlatformSpecific] adds a category trait indicating that a test/test class is non-Linux/-OSX/-FreeBSD/-NetBSD, but the pipelines don't filter those traits away. This means that we end up with runs such as this one, where the Ubuntu and MacOS tests report a non-terminating failure because a test class marked with [PlatformSpecific(TestPlatforms.Windows)] continues to be run and throws an exception during execution with the message "This platform does not support distributed transactions."

Copilot AI review requested due to automatic review settings May 28, 2026 23:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@benrr101
Copy link
Copy Markdown
Contributor Author

This one has gotten too big and messy with copilot spam after a small change. Recreating the PR.

@benrr101 benrr101 closed this May 29, 2026
@github-project-automation github-project-automation Bot moved this from To triage to Done in SqlClient Board May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants