Skip to content

Commit

Permalink
Implement new conformance test executable (#234)
Browse files Browse the repository at this point in the history
## Summary

**This PR adds a new `ConnectConformanceClient` executable which
implements the client-side of the [conformance
service](https://buf.build/connectrpc/conformance/file/main:connectrpc/conformance/v1/service.proto)
and replaces the existing conformance test scaffolding. The conformance
test runner feeds test cases into this executable which subsequently
invokes an RPC on a local service for each case and returns the results
to the runner which validates the output.**

## Details

Connect's [conformance tests
repository](https://github.com/connectrpc/conformance) has moved to a
new conformance testing strategy: It now provides a conformance test
runner that accepts an executable provided by each Connect
implementation which exercises its runtime behavior. The test runner
performs a matrix of hundreds of tests against a local server in order
to validate behaviors with various permutations of protocols, codecs,
etc. This approach allows for developing new test scenarios in one place
(the conformance test runner/repository) and having all those tests run
against each Connect library with little/no changes.

These changes allow us to finally remove the Docker dependency from
local and CI testing. Tests also run more quickly than before, despite
running many more test cases.

**Logs indicate that the test runner is running 632 tests with
URLSession, and another 1,138 tests with SwiftNIO (which has more
combinations since our NIO implementation supports gRPC).**

Note: There are a couple of tests that are explicitly opted-out in the
`opt-outs.txt` file. This is due to the conformance runner being too
strict on base64 encoding for GET requests, and should be fixed in a
future release of the runner.

## Other changes

- Removes `buf.work.yaml` files
- Updates all generated source directories to consistently be named
`GeneratedSources`
- Adds `@unchecked Sendable` to mock interfaces to silence a warning
that appeared when updating them. These must be `@unchecked` because
they are meant to be subclassed in mocks
- Updates some unit tests to use the new conformance Protobuf files
  • Loading branch information
rebello95 authored Jan 17, 2024
1 parent fbc6112 commit 8f82e8b
Show file tree
Hide file tree
Showing 89 changed files with 11,175 additions and 4,643 deletions.
30 changes: 20 additions & 10 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,32 @@ Outputted code will be available in the `out` directories specified by

## Running Tests

A test server is used to run
[conformance](../Tests/ConnectLibraryTests/ConnectConformance) -
integration tests which validate the behavior of the `Connect` library with
various protocols. **Starting the server requires Docker,
so ensure that you have Docker installed before proceeding.**

To start the server and run tests using the command line:
### Conformance Tests

The various Connect implementations across languages leverage a shared
[conformance test repository](https://github.com/connectrpc/conformance) which
contains a test runner that accepts an executable provided by each library
which exercises its runtime behavior. The test runner is responsible for
performing a matrix of hundreds of runtime tests against a local
server in order to validate behaviors with various permutations of
protocols, codecs, etc. Connect-Swift's executable which is compatible with
the conformance runner can be found under
[`Tests/ConformanceClient`](../Tests/ConformanceClient).
To install the runner and run the conformance test suite:

```sh
make test
make installconformancerunner
make testconformance
```

If you prefer to run the tests using Xcode, you can manually start the server:
### Unit Tests

Unit tests live in the [`UnitTests` directory](../Tests/UnitTests)
and can be run using the following command which starts up a local server
and runs the tests:

```sh
make conformanceserverrun
make testunit
```

## Linting
Expand Down
26 changes: 17 additions & 9 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -97,21 +97,29 @@ jobs:
run: |
git update-index --refresh --add --remove
git diff-index --quiet HEAD --
run-connect-tests:
run-conformance-tests:
runs-on: macos-13
steps:
- uses: actions/checkout@v4
- name: Select Xcode version
# https://github.com/actions/runner-images/releases/tag/macos-13%2F20231218.2
run: sudo xcode-select --switch /Applications/Xcode_15.1.app
- name: Set up docker (missing on macOS GitHub runners)
# https://github.com/actions/runner-images/issues/2150
run: |
brew unlink [email protected] && brew link --overwrite [email protected]
brew install docker colima
colima delete && colima start
- name: Run tests
run: make test
- name: Install conformance runner
run: make installconformancerunner
- name: Run conformance tests
run: make testconformance
run-unit-tests:
runs-on: macos-13
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: 1.21.x
- name: Select Xcode version
# https://github.com/actions/runner-images/releases/tag/macos-13%2F20231218.2
run: sudo xcode-select --switch /Applications/Xcode_15.1.app
- name: Run unit tests
run: make testunit
run-swiftlint:
runs-on: ubuntu-latest
container:
Expand Down
5 changes: 3 additions & 2 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ included:
- Plugins
- Tests
excluded:
- Libraries/Connect/Internal/Generated
- Tests/ConnectLibraryTests/Generated
- Libraries/Connect/Internal/GeneratedSources
- Tests/ConformanceClient/GeneratedSources
- Tests/UnitTests/ConnectLibraryTests/GeneratedSources
disabled_rules:
- blanket_disable_command
- cyclomatic_complexity
Expand Down
3 changes: 0 additions & 3 deletions Examples/buf.work.yaml

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// swift-format-ignore-file
//
// Generated by the Swift generator plugin for the protocol buffer compiler.
// Source: grpc/status/v1/status.proto
// Source: proto/grpc/status/v1/status.proto
//
// For information on using the generated types, please see the documentation:
// https://github.com/apple/swift-protobuf/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private extension ProtocolClientConfig {
URLQueryItem(name: "base64", value: "1"),
URLQueryItem(
name: "compression",
value: request.headers[HeaderConstants.contentEncoding]?.first
value: request.headers[HeaderConstants.contentEncoding]?.first ?? "identity"
),
URLQueryItem(name: "connect", value: "v\(ConnectInterceptor.protocolVersion)"),
URLQueryItem(name: "encoding", value: self.codec.name()),
Expand Down
15 changes: 6 additions & 9 deletions Libraries/Connect/Internal/Interceptors/GRPCWebInterceptor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,7 @@ extension GRPCWebInterceptor: UnaryInterceptor {
headers: response.headers,
message: response.message,
trailers: response.trailers,
error: response.error ?? ConnectError.fromGRPCTrailers(
response.headers, code: code
),
error: ConnectError.fromGRPCTrailers(response.headers, code: code),
tracingInfo: response.tracingInfo
))
return
Expand Down Expand Up @@ -212,13 +210,12 @@ private extension Trailers {
}

let trailerName = String(line.prefix(upTo: separatorIndex)).lowercased()
var trailerValue = String(line.suffix(from: separatorIndex + 1))
if trailerValue.hasPrefix(" ") {
trailerValue.removeFirst()
let trailerValues = String(line.suffix(from: separatorIndex + 1))
for value in trailerValues.components(separatedBy: ",") {
trailers[trailerName, default: []].append(
value.trimmingCharacters(in: .whitespaces)
)
}
trailers[trailerName] = trailerValue
.split(separator: ",")
.map { String($0) }
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ extension BidirectionalAsyncStream: BidirectionalAsyncStreamInterface {
func close() {
self.requestCallbacks?.sendClose()
}

func cancel() {
self.requestCallbacks?.cancel()
}
}

// Conforms to the client-only interface since it matches exactly and the implementation is internal
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ extension BidirectionalStream: BidirectionalStreamInterface {
func close() {
self.requestCallbacks.sendClose()
}

func cancel() {
self.requestCallbacks.cancel()
}
}

// Conforms to the client-only interface since it matches exactly and the implementation is internal
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,8 @@ extension ServerOnlyAsyncStream: ServerOnlyAsyncStreamInterface {
func results() -> AsyncStream<StreamResult<Output>> {
return self.bidirectionalStream.results()
}

func cancel() {
self.bidirectionalStream.cancel()
}
}
4 changes: 4 additions & 0 deletions Libraries/Connect/Internal/Streaming/ServerOnlyStream.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ extension ServerOnlyStream: ServerOnlyStreamInterface {
self.bidirectionalStream.send(input)
self.bidirectionalStream.close()
}

func cancel() {
self.bidirectionalStream.cancel()
}
}
15 changes: 13 additions & 2 deletions Libraries/Connect/Internal/Streaming/URLSessionStream.swift
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ final class URLSessionStream: NSObject, @unchecked Sendable {
}
}

func cancel() {
self.task.cancel()
}

func close() {
self.writeStream.close()
}
Expand Down Expand Up @@ -114,10 +118,17 @@ final class URLSessionStream: NSObject, @unchecked Sendable {

self.closedByServer.value = true
if let error = error {
let code = Code.fromURLSessionCode((error as NSError).code)
self.responseCallbacks.receiveClose(
Code.fromURLSessionCode((error as NSError).code),
code,
[:],
error
ConnectError(
code: code,
message: error.localizedDescription,
exception: nil,
details: [],
metadata: [:]
)
)
} else {
self.responseCallbacks.receiveClose(.ok, [:], nil)
Expand Down
13 changes: 3 additions & 10 deletions Libraries/Connect/Internal/Unary/UnaryAsyncWrapper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ actor UnaryAsyncWrapper<Output: ProtobufMessage>: Sendable {
return await withTaskCancellationHandler(operation: {
return await withCheckedContinuation { continuation in
if Task.isCancelled {
continuation.resume(returning: .canceled())
continuation.resume(
returning: .init(code: .canceled, result: .failure(.canceled()))
)
} else {
self.cancelable = self.sendUnary { response in
continuation.resume(returning: response)
Expand All @@ -56,12 +58,3 @@ actor UnaryAsyncWrapper<Output: ProtobufMessage>: Sendable {
})
}
}

private extension ResponseMessage {
static func canceled() -> Self {
return .init(
code: .canceled,
result: .failure(.from(code: .canceled, headers: [:], source: nil))
)
}
}
6 changes: 3 additions & 3 deletions Libraries/Connect/PackageInternal/ConnectError+GRPC.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extension ConnectError {
message: trailers.grpcMessage(),
exception: nil,
details: trailers.connectErrorDetailsFromGRPC(),
metadata: [:]
metadata: trailers
)
}
}
Expand All @@ -47,12 +47,12 @@ private extension Trailers {
func connectErrorDetailsFromGRPC() -> [ConnectError.Detail] {
return self[HeaderConstants.grpcStatusDetails]?
.first
.flatMap { Data(base64Encoded: $0) }
.flatMap { Data(base64Encoded: $0.addingBase64PaddingIfNeeded()) }
.flatMap { data -> Grpc_Status_V1_Status? in
return try? ProtoCodec().deserialize(source: data)
}?
.details
.compactMap { protoDetail in
.map { protoDetail in
return ConnectError.Detail(
// Include only the type name (last component of the type URL)
// to be compatible with SwiftProtobuf's `Google_Protobuf_Any`.
Expand Down
6 changes: 4 additions & 2 deletions Libraries/Connect/PackageInternal/Envelope.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ public enum Envelope {
_ source: Data, using compression: ProtocolClientConfig.RequestCompression?
) -> Data {
var buffer = Data()
if let compression = compression, compression.shouldCompress(source),
let compressedSource = try? compression.pool.compress(data: source)
if !source.isEmpty,
let compression = compression,
compression.shouldCompress(source),
let compressedSource = try? compression.pool.compress(data: source)
{
buffer.append(0b00000001) // 1 byte with the compression bit active
self.write(lengthOf: compressedSource, to: &buffer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,11 @@ extension ProtocolClient: ProtocolClientInterface {
}
}
)
return RequestCallbacks<Input> { requestMessage in
return RequestCallbacks<Input>(cancel: {
pendingRequestCallbacks.enqueue { requestCallbacks in
requestCallbacks.cancel()
}
}, sendData: { requestMessage in
// Wait for the stream to be established before sending data.
pendingRequestCallbacks.enqueue { requestCallbacks in
interceptorChain.executeLinkedInterceptors(
Expand All @@ -383,11 +387,11 @@ extension ProtocolClient: ProtocolClientInterface {
finish: requestCallbacks.sendData
)
}
} sendClose: {
}, sendClose: {
pendingRequestCallbacks.enqueue { requestCallbacks in
requestCallbacks.sendClose()
}
}
})
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ open class URLSessionHTTPClient: NSObject, HTTPClientInterface, @unchecked Senda
self.metricsClosures[urlSessionStream.taskID] = responseCallbacks.receiveResponseMetrics
}
return RequestCallbacks(
cancel: { urlSessionStream.cancel() },
sendData: { data in
do {
try urlSessionStream.sendData(data)
Expand Down Expand Up @@ -184,8 +185,10 @@ extension HTTPURLResponse {
return
}

let headerValue = current.value as? String ?? String(describing: current.value)
headers[headerName] = headerValue.components(separatedBy: ",")
let headerValues = current.value as? String ?? String(describing: current.value)
for value in headerValues.components(separatedBy: ",") {
headers[headerName, default: []].append(value.trimmingCharacters(in: .whitespaces))
}
}
}
}
Expand Down
35 changes: 23 additions & 12 deletions Libraries/Connect/Public/Interfaces/ConnectError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,8 @@ public struct ConnectError: Swift.Error, Sendable {

public init(from decoder: Swift.Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)

// Read the base64-encoded payload and then pad it if needed:
// Base64-encoded strings should be a length that is a multiple of four. If the
// original string is not, it should be padded with "=" to guard against a
// corrupted string.
let encodedPayload = try container.decodeIfPresent(String.self, forKey: .payload) ?? ""
let paddedPayload = encodedPayload.padding(
// Calculate the nearest multiple of 4 that is >= the length of encodedPayload,
// then pad the string to that length.
toLength: ((encodedPayload.count + 3) / 4) * 4,
withPad: "=",
startingAt: 0
)
let paddedPayload = encodedPayload.addingBase64PaddingIfNeeded()
self.init(
type: try container.decodeIfPresent(String.self, forKey: .type) ?? "",
payload: Data(base64Encoded: paddedPayload)
Expand Down Expand Up @@ -144,4 +133,26 @@ extension ConnectError {
)
}
}

public static func canceled() -> Self {
return .init(
code: .canceled, message: "request canceled by client",
exception: nil, details: [], metadata: [:]
)
}
}

extension String {
func addingBase64PaddingIfNeeded() -> Self {
// Base64-encoded strings should be a length that is a multiple of four. If the
// original string is not, it should be padded with "=" to guard against a
// corrupted string.
return self.padding(
// Calculate the nearest multiple of 4 that is >= the length of the string,
// then pad the string to that length.
toLength: ((self.count + 3) / 4) * 4,
withPad: "=",
startingAt: 0
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,8 @@ public protocol BidirectionalAsyncStreamInterface<Input, Output> {

/// Close the stream. No calls to `send()` are valid after calling `close()`.
func close()

/// Cancel the stream and return a canceled code.
/// No calls to `send()` are valid after calling `cancel()`.
func cancel()
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,8 @@ public protocol ClientOnlyAsyncStreamInterface<Input, Output> {

/// Close the stream. No calls to `send()` are valid after calling `close()`.
func close()

/// Cancel the stream and return a canceled code.
/// No calls to `send()` are valid after calling `cancel()`.
func cancel()
}
Loading

0 comments on commit 8f82e8b

Please sign in to comment.