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

niklasad1 · 2021-06-24T14:26:55Z

We have been bitten by that the CI just checks all crates compiles in the workspace (shares Cargo.lock) and features.

Thus, if a feature for an individual is not enabled for specific crate then this is not detected by CI so this PR changes that each crate using its manifest instead.

In addition:

Fixes the ws-server to compile outside the workspace
Added to run CI on macos and Windows.

We have bitten by these a few times now with that some features are leaked from the workspace which makes it compile in the workspace but not using it's own Cargo.toml.

niklasad1 · 2021-06-24T15:11:32Z

ws-server/src/tests.rs

@@ -173,8 +173,6 @@ async fn can_set_max_connections() {
 	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\" }))");


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.

.github/workflows/ci.yml

jsdw · 2021-06-24T15:28:16Z

.github/workflows/ci.yml

@@ -74,8 +74,68 @@ jobs:
          command: check
          args: --manifest-path http-client/Cargo.toml --no-default-features --features tokio02


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?

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.

Thanks, that all makes sense now :)

jsdw · 2021-06-24T15:31:05Z

.github/workflows/ci.yml

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


Do we need to have a build step before the test step? (I'll need to look into these rust github actions 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.

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"!

jsdw · 2021-06-24T15:36:19Z

ws-server/Cargo.toml

@@ -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"] }


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

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

Aah gotcha! Thanks for the explanation!

jsdw

Looks good to me; I have a few questions but nothing major and mainly just for my own curiosity!

dvdplm · 2021-06-25T08:56:13Z

ws-server/src/tests.rs

+	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);


Nice, ty for fixing this.

* master: (21 commits) New proc macro (#387) Streaming RpcParams parsing (#401) Set allowed Host header values (#399) Synchronization-less async connections in ws-server (#388) [ws server]: terminate already established connection(s) when the server is stopped (#396) feat: customizable JSON-RPC error codes via new enum variant on `CallErrror` (#394) [ci]: test each individual crate's manifest (#392) Add a way to stop servers (#386) [jsonrpsee types]: unify a couple of types + more tests (#389) Update roadmap link in readme (#390) Cross-origin protection (#375) Method aliases + RpcModule: Clone (#383) Use criterion's async bencher (#385) Async/subscription benches (#372) send text (#374) Fix link to ws server in README.md (#373) Concat -> simple push (#370) Add missing `rt` feature (#369) Release prep for v0.2 (#368) chore(scripts): publish script (#354) ...

niklasad1 added 4 commits June 24, 2021 16:13

[ci]: test each crate outside workspace

e683cec

We have bitten by these a few times now with that some features are leaked from the workspace which makes it compile in the workspace but not using it's own Cargo.toml.

[ci]: add tests for macos and windows

46b3ec5

add missed WsServer and HttpServer

b66e67b

[ws server]: fix features

433a617

niklasad1 requested review from dvdplm and jsdw June 24, 2021 14:40

niklasad1 added 2 commits June 24, 2021 16:49

debug failure in CI

be54762

remove platform dependent assertion

8edee85

niklasad1 commented Jun 24, 2021

View reviewed changes

fix nit; proc-macros is proc-macros

7e94aff

jsdw reviewed Jun 24, 2021

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

jsdw reviewed Jun 24, 2021

View reviewed changes

jsdw approved these changes Jun 24, 2021

View reviewed changes

niklasad1 added 2 commits June 24, 2021 17:53

restore removed assertion

1a229d3

remove whitespaces

93a936b

dvdplm approved these changes Jun 25, 2021

View reviewed changes

dvdplm merged commit 2ca8355 into master Jun 25, 2021

dvdplm deleted the na-fix-ci branch June 25, 2021 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

niklasad1 commented Jun 24, 2021 •

edited

Loading

niklasad1 Jun 24, 2021 •

edited

Loading

jsdw Jun 24, 2021

niklasad1 Jun 24, 2021

jsdw Jun 25, 2021

jsdw Jun 24, 2021

niklasad1 Jun 24, 2021

jsdw Jun 25, 2021

jsdw Jun 24, 2021

niklasad1 Jun 24, 2021 •

edited

Loading

jsdw Jun 25, 2021

jsdw left a comment

dvdplm Jun 25, 2021

		@@ -74,8 +74,68 @@ jobs:
		command: check
		args: --manifest-path http-client/Cargo.toml --no-default-features --features tokio02

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

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

Conversation

niklasad1 commented Jun 24, 2021 • edited Loading

niklasad1 Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Jun 24, 2021 •

edited

Loading

niklasad1 Jun 24, 2021 •

edited

Loading

niklasad1 Jun 24, 2021 •

edited

Loading