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

[ci]: test each individual crate's manifest #392

Merged
merged 9 commits into from
Jun 25, 2021
Merged

[ci]: test each individual crate's manifest #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e683cec
[ci]: test each crate outside workspace
niklasad1 Jun 24, 2021
46b3ec5
[ci]: add tests for macos and windows
niklasad1 Jun 24, 2021
b66e67b
add missed `WsServer` and `HttpServer`
niklasad1 Jun 24, 2021
433a617
[ws server]: fix features
niklasad1 Jun 24, 2021
be54762
debug failure in CI
niklasad1 Jun 24, 2021
8edee85
remove platform dependent assertion
niklasad1 Jun 24, 2021
7e94aff
fix nit; proc-macros is proc-macros
niklasad1 Jun 24, 2021
1a229d3
restore removed assertion
niklasad1 Jun 24, 2021
93a936b
remove whitespaces
niklasad1 Jun 24, 2021
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
132 changes: 124 additions & 8 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ jobs:
override: true
components: clippy, rustfmt

- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0
- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0

- name: Cargo fmt
uses: actions-rs/cargo@v1.0.3
Expand Down Expand Up @@ -59,10 +59,10 @@ jobs:
toolchain: stable
override: true

- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0
- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0

- name: Cargo check all targets
- name: Cargo check all targets (use Cargo.toml in workspace)
uses: actions-rs/cargo@v1.0.3
with:
command: check
Expand All @@ -74,8 +74,68 @@ jobs:
command: check
args: --manifest-path http-client/Cargo.toml --no-default-features --features tokio02
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of checking every crate individually, could we just run cargo check without arguments? My experience is that that checks every crate in the workspace by default (though you can also add --workspace to, I assume, be sure).

Or do we need to check individually because http-client needs the tokio02 feature setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we did do cargo check --workspace before this PR but that's the problem and why I added cargo check --manifest-path crate, see my comment below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that all makes sense now :)


- name: Cargo check HTTP client
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path http-client/Cargo.toml

- name: Cargo check HTTP server
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path http-server/Cargo.toml

- name: Cargo check WS client
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path ws-client/Cargo.toml

- name: Cargo check WS client with tokio02
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path ws-client/Cargo.toml --no-default-features --features tokio02

- name: Cargo check WS server
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path ws-server/Cargo.toml

- name: Cargo check types
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path types/Cargo.toml

- name: Cargo check utils
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path utils/Cargo.toml

- name: Cargo check proc macros
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path proc-macros/Cargo.toml

- name: Cargo check test utils
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path test-utils/Cargo.toml

- name: Cargo check examples
uses: actions-rs/cargo@v1.0.3
with:
command: check
args: --manifest-path examples/Cargo.toml

tests:
name: Run tests
name: Run tests Ubuntu
runs-on: ubuntu-latest
steps:
- name: Checkout sources
Expand All @@ -88,8 +148,64 @@ jobs:
toolchain: stable
override: true

- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0
- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0

- name: Cargo build
uses: actions-rs/cargo@v1.0.3
with:
command: build
args: --workspace

- name: Cargo test
uses: actions-rs/cargo@v1.0.3
with:
command: test

tests_macos:
name: Run tests macos
runs-on: macos-latest
steps:
- name: Checkout sources
uses: actions/checkout@v2.3.4

- name: Install Rust stable toolchain
uses: actions-rs/toolchain@v1.0.7
with:
profile: minimal
toolchain: stable
override: true

- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0

- name: Cargo build
uses: actions-rs/cargo@v1.0.3
with:
command: build
args: --workspace
Comment on lines +182 to +186
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to have a build step before the test step? (I'll need to look into these rust github actions more!)

Copy link
Member Author

Choose a reason for hiding this comment

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

no you are right, we don't have too but the previous checks only run cargo check so we have want make sure nothing weird happens when building the binary but it could be another action for clarity I suppose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My first thought was that cargo test builds the binary anyway, but of course it builds it with cfg(test) enabled, so I can see why a separate build step is useful to make sure it works "normally"!


- name: Cargo test
uses: actions-rs/cargo@v1.0.3
with:
command: test

tests_windows:
name: Run tests Windows
runs-on: windows-latest
steps:
- name: Checkout sources
uses: actions/checkout@v2.3.4

- name: Install Rust stable toolchain
uses: actions-rs/toolchain@v1.0.7
with:
profile: minimal
toolchain: stable
override: true

- name: Rust Cache
uses: Swatinem/rust-cache@v1.3.0

- name: Cargo build
uses: actions-rs/cargo@v1.0.3
Expand Down
14 changes: 10 additions & 4 deletions test-utils/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use futures_util::{
stream::{self, StreamExt},
};
use serde::{Deserialize, Serialize};
use soketto::handshake;
use soketto::handshake::{server::Response, Server};
use soketto::handshake::{self, server::Response, Error as SokettoError, Server};
use std::io;
use std::net::SocketAddr;
use std::time::Duration;
use tokio::net::TcpStream;
Expand Down Expand Up @@ -63,15 +63,21 @@ impl std::fmt::Debug for WebSocketTestClient {
}

impl WebSocketTestClient {
pub async fn new(url: SocketAddr) -> Result<Self, Error> {
pub async fn new(url: SocketAddr) -> Result<Self, SokettoError> {
let socket = TcpStream::connect(url).await?;
let mut client = handshake::Client::new(BufReader::new(BufWriter::new(socket.compat())), "test-client", "/");
match client.handshake().await {
Ok(handshake::ServerResponse::Accepted { .. }) => {
let (tx, rx) = client.into_builder().finish();
Ok(Self { tx, rx })
}
r => Err(format!("WebSocketHandshake failed: {:?}", r).into()),
Ok(handshake::ServerResponse::Redirect { .. }) => {
Err(SokettoError::Io(io::Error::new(io::ErrorKind::Other, "Redirection not supported in tests")))
}
Ok(handshake::ServerResponse::Rejected { .. }) => {
Err(SokettoError::Io(io::Error::new(io::ErrorKind::Other, "Rejected")))
}
Err(err) => Err(err),
}
}

Expand Down
2 changes: 1 addition & 1 deletion ws-server/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ documentation = "https://docs.rs/jsonrpsee-ws-server"
[dependencies]
thiserror = "1"
futures-channel = "0.3.14"
futures-util = { version = "0.3.14", default-features = false, features = ["io"] }
futures-util = { version = "0.3.14", default-features = false, features = ["io", "async-await-macro"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: It doesn't look like we've added any code that requires this feature; do we want it?

Copy link
Member Author

@niklasad1 niklasad1 Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This fixes the crate to "compile" standalone i.e, outside the workspace.

It was introduced in another PR, thus it doesn't compile standalone such as if you would do cd ws-server && cargo check outside the workspace it doesn't work. Because, features in Rust are additive (the union of all features) so if another crate adds that feature it's enabled for all in workspace if that makes sense.

TLDR; on master cd ws-server && cargo check doesn't work but cargo build -p ws-server works :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aah gotcha! Thanks for the explanation!

jsonrpsee-types = { path = "../types", version = "0.2.0" }
jsonrpsee-utils = { path = "../utils", version = "0.2.0", features = ["server"] }
log = "0.4"
Expand Down
10 changes: 6 additions & 4 deletions ws-server/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,12 @@ async fn can_set_max_connections() {
assert!(conn2.is_ok());
// Third connection is rejected
assert!(conn3.is_err());
let err = conn3.unwrap_err();
assert!(err.to_string().contains("WebSocketHandshake failed"));
assert!(err.to_string().contains("Connection reset by peer"));
// Err(Io(Os { code: 54, kind: ConnectionReset, message: \"Connection reset by peer\" }))");
Copy link
Member Author

@niklasad1 niklasad1 Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

windows returns https://support.microsoft.com/en-us/topic/an-application-may-receive-the-10054-error-when-the-application-receives-data-from-a-connection-on-a-computer-that-is-running-windows-7-or-windows-server-2008-r2-if-a-tdi-filter-driver-is-installed-73fd74a9-51c2-3663-e965-e1187b1cfc92

so let's remove the assertion it would be better if we used io::Error in the test client I suppose then we could actually match on the kind but as this is Box<Error> it doesn't work without downcasting.


let err = match conn3 {
Err(soketto::handshake::Error::Io(err)) => err,
_ => panic!("Invalid error kind; expected std::io::Error"),
};
assert_eq!(err.kind(), std::io::ErrorKind::ConnectionReset);
Comment on lines +175 to +179
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, ty for fixing this.


// Decrement connection count
drop(conn2);
Expand Down