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.

Sign up for GitHub

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

Jump to bottom

Remove warning about non-serializable test data #6102

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Remove warning about non-serializable test data #6102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tracer/test/Datadog.Trace.Tests/Agent/MessagePack/SpanMetaStructTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public class SpanMetaStructTests
}
};

[MemberData(nameof(Data))]
[MemberData(nameof(Data), DisableDiscoveryEnumeration = true)]
Copy link
Member

@lucaspimentel lucaspimentel Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we want to fix the underlying issue instead of disabling the warning? The warning says "falling back to single test case" which makes me think it will only run one tests case instead of all of them.

See #5089

Copy link
Contributor Author

@bouwkast bouwkast Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean, maybe? But I think they are useless warnings.

which makes me thing it will only run on tests case instead of all of them.

I don't think so, the tests all run locally on the CLI and via the VS Test Explorer.

What is happening is that xunit (I think or maybe the test host) is attempting to enumerate all of the individual theory data lines just so that you could run an individual Theory data by itself instead of the entire test.

That is what the Non-serializable data ('System.Object[]') found for 'Datadog.Trace.Tests.Tagging.ActivityTagsTests.Tags_ShouldBe_PlacedInMetricsOrMeta'; falling back to single test case. is ultimately meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see that we have been fixing them to be serializable previously

But I'll swap to fix it to be consistent.

I am curious if that enumeration is slowing our tests down 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relevant comment in the xunit code: https://github.com/xunit/xunit/blob/414e2c88661d9122f1c3e9b595839647a3188119/src/xunit.v3.core/Framework/TheoryDiscoverer.cs#L190-L196

Copy link
Member

Choose a reason for hiding this comment

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

You're right it still runs all the cases at run time, so meh. Thanks for looking into this!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the analysis is 100 correct, it just gives a worse local dev experience, I only learned that after raising an issue for it here 😄 xunit/xunit#2866

I am curious if that enumeration is slowing our tests down

My strong suspicion is that it's a drop in the ocean and not worth worrying about 😃

[Theory]
public static void GivenAEncodedSpanWithMetaStruct_WhenDecoding_ThenMetaStructIsCorrectlyDecoded(List<Tuple<string, object?>> dataToEncode)
{
Expand Down
4 changes: 2 additions & 2 deletions tracer/test/Datadog.Trace.Tests/Tagging/ActivityTagsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public enum TagKind
// TODO what about an array of object that contains string and numeric objects?

[Theory]
[MemberData(nameof(TagData))]
[MemberData(nameof(TagData), DisableDiscoveryEnumeration = true)]
public void Tags_ShouldBe_PlacedInMetricsOrMeta(string tagKey, object tagValue, TagKind expectedTagKind)
{
var activityMock = new Mock<IActivity5>();
Expand All @@ -97,7 +97,7 @@ public void Tags_ShouldBe_PlacedInMetricsOrMeta(string tagKey, object tagValue,
}

[Theory]
[MemberData(nameof(ArrayTagData))]
[MemberData(nameof(ArrayTagData), DisableDiscoveryEnumeration = true)]
public void ArrayedTags_ShouldBe_PlacedInMeta(string tagKey, object tagValue, TagKind expectedTagKind, Dictionary<string, object> expectedTagValues)
{
var activityMock = new Mock<IActivity5>();
Expand Down
Loading