Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Fix non-serializable tests #1820

Merged
merged 6 commits into from
Apr 2, 2018
Merged

Fix non-serializable tests #1820

merged 6 commits into from
Apr 2, 2018

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Apr 1, 2018

Also fix ignoring empty string on CloseMessage

#1649

Fix ignoring empty string on CloseMessage
@JamesNK
Copy link
Member Author

JamesNK commented Apr 1, 2018

Outstanding are a couple of tests with the WebSocketMessageType enum parameter that get this error. Any idea why? Other parameterized tests have enum arguments and are fine.

https://github.com/aspnet/SignalR/blob/dev/test/Microsoft.AspNetCore.Http.Connections.Tests/WebSocketsTests.cs#L27-L111

@@ -1686,15 +1688,16 @@ public static IEnumerable<object[]> StreamingMethodAndHubProtocols
{
get
{

Copy link
Member

Choose a reason for hiding this comment

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

super nit: extra space.

@davidfowl
Copy link
Member

Outstanding are a couple of tests with the WebSocketMessageType enum parameter that get this error. Any idea why? Other parameterized tests have enum arguments and are fine.

Nope, thats odd.

@JamesNK
Copy link
Member Author

JamesNK commented Apr 2, 2018

Creating an almost identical enum and using that instead fixed it 😕

Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

No more warnings and proper listing of Theories in Test Explorer/Resharper? I'm in for sure.

public enum TestWebSocketMessageType
{
Binary = 1,
Text = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth filing a bug, but I don't know exactly where it would go. I think maybe it's xunit here? WebSocketMessageType doesn't look any different from this enum really: https://source.dot.net/#System.Net.WebSockets/System/Net/WebSockets/WebSocketMessageType.cs,17aa513fb8c7e9fc

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like it's a problem on desktop framework when the enum is in the GAC. https://twitter.com/anurse/status/980825964757176320

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Another reason to hate the GAC

@@ -254,5 +269,25 @@ private static byte[] FormatMessageToArray(byte[] message)
TextMessageFormatter.WriteRecordSeparator(output);
return output.ToArray();
}

public class ProtocolTestData1
Copy link
Contributor

Choose a reason for hiding this comment

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

ProtocolTestData1?

[InlineData(WebSocketMessageType.Binary)]
public async Task ReceivedFramesAreWrittenToChannel(WebSocketMessageType webSocketMessageType)
[InlineData(TestWebSocketMessageType.Text)]
[InlineData(TestWebSocketMessageType.Binary)]

Choose a reason for hiding this comment

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

As I suggested on Twitter, I believe we convert string data into enum values. I don't know if nameof supports enum values (to help preserve values across renames), but given your work-around, you've already given up on rename refactor support anyway...

Copy link
Contributor

Choose a reason for hiding this comment

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

nameof should work here actually, so we could try that.

@JamesNK JamesNK merged commit 3460d44 into dev Apr 2, 2018
@bradwilson
Copy link

Did auto-conversion from string test data to Enum parameter not work correctly?

@JamesNK JamesNK deleted the jamesnk/non-serializable-tests branch April 6, 2018 05:56
dougbu added a commit to aspnet/Mvc that referenced this pull request Oct 28, 2018
- avoid "Skipping test case with duplicate ID" messages
  - xUnit gets confused when `[Theory]`s are overridden
- avoid "falling back to single test case" messages
  - fix inspired by aspnet/SignalR#1820
dougbu added a commit to aspnet/Mvc that referenced this pull request Oct 29, 2018
- avoid "Skipping test case with duplicate ID" messages
  - xUnit gets confused when `[Theory]`s are overridden
- avoid "falling back to single test case" messages
  - fix inspired by aspnet/SignalR#1820
dougbu added a commit to aspnet/Mvc that referenced this pull request Oct 29, 2018
- avoid "Skipping test case with duplicate ID" messages
  - xUnit gets confused when `[Theory]`s are overridden
- avoid "falling back to single test case" messages
  - fix inspired by aspnet/SignalR#1820
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants