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

Websockets transport now creates correct url #268

Merged
merged 3 commits into from
Mar 7, 2017
Merged

Conversation

mikaelm12
Copy link
Contributor

Addresses: #266

@dnfclas
Copy link

dnfclas commented Mar 6, 2017

@mikaelm12,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@@ -37,7 +37,7 @@ public EndToEndTests(ServerFixture serverFixture)

[ConditionalFact]
[OSSkipCondition(OperatingSystems.Windows, WindowsVersions.Win7, WindowsVersions.Win2008R2, SkipReason = "No WebSockets Client for this platform")]
public async Task WebSocketsTest()
public async Task BareWebSocketsTest()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why the test name was changed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be a little more explicit that this test is using bare websockets and not testing the transports. I can change this back

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, change it back. Other than that it now looks good.

yield return new object[] { new string('A', 5 * 1024 * 1024 + 32)};
yield return new object[] { new string('A', 5 * 1024), new WebSocketsTransport()};
yield return new object[] { new string('A', 5 * 1024 * 1024 + 32), new WebSocketsTransport() };
yield return new object[] { new string('A', 5 * 1024), new LongPollingTransport(new HttpClient())};
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem right (given the name of the property). If needed at all (and I am not convinced it is) you should create a getter that creates a Cartesian product of what before was MessageSizesData and Transports

}
}

[ConditionalTheory]
[OSSkipCondition(OperatingSystems.Windows, WindowsVersions.Win7, WindowsVersions.Win2008R2, SkipReason = "No WebSockets Client for this platform")]
[MemberData(nameof(MessageSizesData))]
public async Task ConnectionCanSendAndReceiveDifferentMessageSizesWebSocketsTransport(string message)
public async Task ConnectionCanSendAndReceiveDifferentMessageSizesWebSocketsTransport(string message, ITransport transport)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test (as indicated by its name) is used to test the logic we have in the WebSockets transport to combine messages which don't fit the buffer/have endOfMessage flag set to false. As such this test is specific to WebSocketsTransport and doesn't have to be run against other transports. We already have a test above that test that sending and receiving data using different transports works.

Copy link
Contributor

@moozzyk moozzyk left a comment

Choose a reason for hiding this comment

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

Just revert the BareWebSocketsTest change

@mikaelm12
Copy link
Contributor Author

Hmm. Travis Failure https://travis-ci.org/aspnet/SignalR/jobs/208377625#L2744
Any help on this @anurse

@analogrelay
Copy link
Contributor

*sigh* it's something timing out on Travis yet again. I can't figure out why these tests are so flaky only on Travis. It's got to be something about slow machines and task scheduling there, but it's almost impossible to reproduce on any of my normal machines. For now, restarting the build (which I just did) normally works.

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