[EFCore] Fix integration test#3626
Conversation
Update expected span name after changes in open-telemetry#3592.
github-actions
Bot
added
the
comp:instrumentation.entityframeworkcore
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3626 +/- ##
==========================================
- Coverage 71.41% 71.29% -0.12%
==========================================
Files 443 453 +10
Lines 17519 17570 +51
==========================================
+ Hits 12511 12527 +16
- Misses 5008 5043 +35 Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Skip the old conventions test for SQL Server.
There was a problem hiding this comment.
Pull request overview
This PR fixes an Entity Framework Core integration test that was failing on Linux after changes in PR #3592. The update aligns the test expectations with new semantic convention naming for SQL Server database systems.
Key Changes
- Removed the SQL Server test case using the old naming convention (
useNewConventions=falsewith "mssql") - Retained only the SQL Server test case using the new naming convention (
useNewConventions=truewith "microsoft.sql_server")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Any chance to check why the EFCore tests was not triggered while code shared with SqlClient instrumentation was changed? |
|
There's no shared code, but the EFCore tests depend on the SqlClient instrumentation project for the test which validates they work together: |
|
Maybe it is worth to add this: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 5f94c12c..f2894fb9 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -56,7 +56,7 @@ jobs:
instrumentation-cassandra: ['*/OpenTelemetry.Instrumentation.Cassandra*/**', '!**/*.md']
instrumentation-confluentkafka: ['*/OpenTelemetry.Instrumentation.ConfluentKafka*/**', 'examples/kafka/**', '!**/*.md']
instrumentation-elasticsearchclient: ['*/OpenTelemetry.Instrumentation.ElasticsearchClient*/**', '!**/*.md']
- instrumentation-entityframeworkcore: ['*/OpenTelemetry.Instrumentation.EntityFrameworkCore*/**', '!**/*.md']
+ instrumentation-entityframeworkcore: ['*/OpenTelemetry.Instrumentation.EntityFrameworkCore*/**', '*/OpenTelemetry.Instrumentation.SqlClient/**', '!**/*.md']
instrumentation-eventcounters: ['*/OpenTelemetry.Instrumentation.EventCounters*/**', 'examples/event-counters/**', '!**/*.md']
instrumentation-grpccore: ['*/OpenTelemetry.Instrumentation.GrpcCore*/**', '!**/*.md']
instrumentation-grpcnetclient: ['*/OpenTelemetry.Instrumentation.GrpcNetClient*/**', '!**/*.md']And fix existing test case? |
I started doing that at first, but it got complicated very fast with "use old conventions, but only for these bits, not for those bits" so I figured it wasn't worth the extra complexity so just removed the test case. Otherwise it's no longer binary ( |
Also run the EFCore tests if SqlClient is changed.
|
@Kielek Are you happy to approve this so we can get main fixed and unblock other PRs? |
Kielek
left a comment
Kielek
left a comment
There was a problem hiding this comment.
@martincostello, it should be fine. Sorry for delay - last couple days I need to take care of some internal tasks.
Changes
Remove integration test for SQL Server using the old conventions after changes in #3592.
The test only runs on Linux, so is easy to overlook if not changing the EFCore files and working on Windows locally.
Merge requirement checklist
AppropriateCHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)