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

Added tests for server & bi-directional streaming #183

Merged
merged 4 commits into from
Mar 13, 2018
Merged

Added tests for server & bi-directional streaming #183

merged 4 commits into from
Mar 13, 2018

Conversation

cvanderschuere
Copy link
Contributor

Following up on #144, these tests verify that #147 fixed the problem.

@MrMage
Copy link
Collaborator

MrMage commented Mar 12, 2018

Thanks a lot for this, it's great to see more cases of the API's usage covered with tests. I'll leave a full review to @timburks (he's the maintainer and knows this portion of the code better).

I think that it would be nicer to just have one or two extra test cases in EchoTests (possibly with a slightly customized version of EchoProvider), but I think the current generated server-side code would just "swallow" errors like that instead of returning them, so I guess this can't be tested in EchoTests right now.

Also, would you mind adapting your indentation to match the project's (2-spaces, no tabs)? Unfortunately, swift package generate-xcodeproj always resets that setting to the system defaults, maybe I can create a script to automatically change the defaults for the generated project.

@@ -85,7 +85,7 @@ class GeneratorOptions {
}

static func parseParameter(string: String?) -> [(key: String, value: String)] {
guard let string = string, string.characters.count > 0 else {
guard let string = string, string.isEmpty else {
Copy link
Member

Choose a reason for hiding this comment

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

This isEmpty condition should be inverted, i.e. it should be !string.isEmpty instead of string.isEmpty. Fortunately, our plugin test (make test-plugin) caught it!

If you don't mind, please update that in your PR. When the tests pass, I'll merge it.

Thank you!

@timburks
Copy link
Member

With the inverted guard condition fixed, I'm still seeing a test failure on Linux:

/home/travis/build/grpc/grpc-swift/Tests/SwiftGRPCTests/GRPCTests.swift:253: error: gRPCTests.testConnectivitySecure : XCTAssertEqual failed: ("ok") is not equal to ("resourceExhausted") - 
Test Case 'gRPCTests.testConnectivitySecure' failed (0.045 seconds)

@cvanderschuere
Copy link
Contributor Author

@timburks I cannot figure out what's going on here. There seems to be some sort of race condition, but nothing is standing out to me. I also cannot reproduce this error on OSX.

@timburks
Copy link
Member

@cvanderschuere Have you tested on Linux? I'll try to reproduce this in a Linux VM.

@timburks
Copy link
Member

@cvanderschuere I can confirm that the test fails on Ubuntu 16.04. The "ok" result is being set on this line which suggests that the operation group failed (and .ok should be something else, possibly .unknown). I'll continue investigating. Thanks for creating this test!

@timburks
Copy link
Member

timburks commented Mar 13, 2018

@cvanderschuere I don't think this line does what your comment suggests that it will do. @MrMage recently added that queue and from reading Call.swift, I think it only synchronizes message sends and not call starts.

Also, I think we need to add sem.wait() here to keep us from ending the call before the server has closed the connection.

Update: the test seems to consistently pass for me when I delete call.messageQueueEmpty.wait() and add sem.wait() at the end of func callBiDiStream(). And I think that this line can also be deleted.

@cvanderschuere
Copy link
Contributor Author

@timburks Thanks for the help! I certainly misunderstood what messageQueueEmpty was doing and was stuck in not being able to reproduce the issues.

I made the changes you suggested and all looks to be well.

@timburks timburks merged commit 13351ff into grpc:master Mar 13, 2018
@timburks
Copy link
Member

@cvanderschuere Thanks for working through this!

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