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

Tests | Split MultipleResultsTest, clean up console logging #3060

Merged
merged 8 commits into from
Mar 6, 2025

Conversation

edwardneal
Copy link
Contributor

This starts to adjust the way the SqlClient tests use the console stdout. It contains two changes.

MultipleResultsTest rework

This was one large method. It would temporarily replace the console stdout, log to it, then manually perform a diff between the stdout and a file bundled with the project.

I've modified this, splitting the large method into three tests (ExecuteNonQuery, ExecuteReader, ExecuteScalar.) These tests are designed to prove the way that SqlClient handles informational messages, exceptions and result sets when MARS is enabled.

Console stdout rework

I've noticed a few instances where standard output from the TDS servers get mixed in with the test results logged in the CI pipeline. This output isn't useful, even when the tests fail; I've disabled it in a few areas.

@@ -76,7 +76,6 @@ public void CustomActiveDirectoryProviderTest_AppClientId_DeviceFlowCallback()

private Task CustomDeviceFlowCallback(DeviceCodeResult result)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would help readability if we remove this method. It's not doing anything useful now that the logging is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've removed it. Can you trigger another CI run please?

@mdaigle
Copy link
Contributor

mdaigle commented Feb 19, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.21%. Comparing base (0be2756) to head (f085005).
Report is 18 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (0be2756) and HEAD (f085005). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (0be2756) HEAD (f085005)
addons 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3060      +/-   ##
==========================================
- Coverage   72.81%   66.21%   -6.61%     
==========================================
  Files         282      276       -6     
  Lines       59112    58802     -310     
==========================================
- Hits        43045    38933    -4112     
- Misses      16067    19869    +3802     
Flag Coverage Δ
addons ?
netcore 69.17% <ø> (-6.35%) ⬇️
netfx 64.86% <ø> (-6.28%) ⬇️

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.

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

This is awesome, I love anything that breaks up complicated tests and cleans up the console output on the tests. 🚀

Would love to see the few comments I made addressed, but I also won't hold things up if you don't want to address them.

@benrr101
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@benrr101 benrr101 merged commit 87b5435 into dotnet:main Mar 6, 2025
123 checks passed
@edwardneal edwardneal deleted the tests/console-logging branch March 6, 2025 19:52
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.

3 participants