Skip to content

Move Test Config to Jsonc#4297

Open
benrr101 wants to merge 10 commits into
mainfrom
dev/russellben/config-jsonc
Open

Move Test Config to Jsonc#4297
benrr101 wants to merge 10 commits into
mainfrom
dev/russellben/config-jsonc

Conversation

@benrr101
Copy link
Copy Markdown
Contributor

@benrr101 benrr101 commented May 20, 2026

Description

As a byproduct of the PR pipeline, I got annoyed with the red squigglies under the comments in the test config.json file. Technically speaking jsonc files support comments while json does not. So, this small PR does the following:

  • Changes the config.default.json file to config.default.jsonc
  • Rewrites the Config.Load logic so that it tries the environment variable path first, config.jsonc second, then config.json before failing.
  • Makes Config class #nullable
  • Sync'd the config.default.json file with the fields that actually exist (and removed unused fields).
  • Remove dependency on Newtonsoft.Json from TestUtilities and push it back to ManualTests.

🤖

Used 🤖 to find remaining references to config.json and replace with references to config.jsonc

Issues

N/A

Testing

Everything builds, but I want to validate with a PR run before taking it out of draft state.

Copilot AI review requested due to automatic review settings May 20, 2026 03:44
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board May 20, 2026
public string DNSCachingConnString = null;
public string DNSCachingServerCR = null; // this is for the control ring
public string DNSCachingServerTR = null; // this is for the tenant ring
public bool IsAzureSynapse = false; // True for Azure Data Warehouse/Synapse
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 removed as it is not used.

public string AliasName = null;

public static Config Load(string configPath = @"config.json")
public static Config Load(string configPath)
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 added as an escape hatch for preserving the behavior of ExtUtilities project. I have plans to remove the ExtUtilities project, so this method will hopefully go away someday.

"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.

These fields exist in Config class, but did not in the json file.

"SupportsIntegratedSecurity": true,
"LocalDbAppName": "",
"LocalDbSharedInstanceName": "",
"SupportsFileStream": 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 field did not exist in the Config class.

"DNSCachingServerTR": "",
"IsDNSCachingSupportedCR": false,
"IsDNSCachingSupportedTR": false,
"IsAzureSynapse": 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 field existed in the Config class, but was removed since it's unused.

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings May 20, 2026 17:15
@benrr101 benrr101 added this to the 7.1.0-preview2 milestone May 20, 2026

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings May 20, 2026 20:01

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings May 20, 2026 23:56

This comment was marked as outdated.

@benrr101 benrr101 force-pushed the dev/russellben/config-jsonc branch from 5885bb9 to 57e9b41 Compare May 21, 2026 00:09
Copilot AI review requested due to automatic review settings May 21, 2026 15:51
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.

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.

Comment thread eng/pipelines/common/templates/steps/update-config-file-step.yml
Comment thread eng/pipelines/kerberos/sqlclient-kerberos.yml
Comment thread TESTGUIDE.md
@benrr101 benrr101 marked this pull request as ready for review May 27, 2026 17:52
@benrr101 benrr101 requested a review from a team as a code owner May 27, 2026 17:52
Copilot AI review requested due to automatic review settings May 27, 2026 17:52
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.

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

Comment thread TESTGUIDE.md

```bash
MDS_TEST_CONFIG=/path/to/config.json dotnet build -t:TestSqlClientManual -p:TestSet=2
MDS_TEST_CONFIG=/path/to/config.jsonc dotnet build -t:TestSqlClientManual -p:TestSet=2
<!-- Copy config file ================================================ -->
<Target Name="CopyConfig" BeforeTargets="Compile">
<Copy SourceFiles="config.default.json" DestinationFiles="config.json" Condition="!Exists('config.json')" />
<Copy SourceFiles="config.default.jsonc" DestinationFiles="config.jsonc" Condition="!Exists('config.jsonc')" />
@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.95%. Comparing base (c724554) to head (37b4f64).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4297      +/-   ##
==========================================
- Coverage   66.57%   65.95%   -0.63%     
==========================================
  Files         284      279       -5     
  Lines       43301    66219   +22918     
==========================================
+ Hits        28827    43673   +14846     
- Misses      14474    22546    +8072     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 65.95% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@benrr101 benrr101 mentioned this pull request May 28, 2026
@paulmedynski paulmedynski moved this from To triage to In review in SqlClient Board May 29, 2026
@paulmedynski paulmedynski added the Area\Tests Issues that are targeted to tests or test projects label May 29, 2026
@paulmedynski paulmedynski self-assigned this May 29, 2026
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Looks great! I had updated VS Code to treat .json and .jsonc files as JSON with Comments, so I wasn't seeing any red squigglies :)

type: boolean
default: false

- name: SupportsFileStream
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This parameter is no unused and can be removed.

@github-project-automation github-project-automation Bot moved this from In review to Waiting for customer 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\Tests Issues that are targeted to tests or test projects

Projects

Status: Waiting for customer

Development

Successfully merging this pull request may close these issues.

4 participants