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

Jump to bottom

wip: socks 5 test #132

Closed
wants to merge 18 commits into from
Closed

wip: socks 5 test #132

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e10f6ad
enable test and extract vars
mario-s Apr 30, 2022
3fc43a9
pattern matching for vars
mario-s May 2, 2022
ed8a0ee
add test-with
mario-s May 2, 2022
aaf0a90
reduce ws
mario-s May 2, 2022
31c4e06
rename VAR
mario-s May 2, 2022
f1fabac
add doc
mario-s May 2, 2022
a07e758
typo
mario-s May 2, 2022
63cfd4d
formart
mario-s May 2, 2022
fd50a51
ws
mario-s May 2, 2022
a95dacd
take formart from main
mario-s May 2, 2022
8e714f0
remove redundant clone
mario-s May 2, 2022
3a044e5
remove obsolete comment
mario-s May 2, 2022
a512ba9
replace lib with macro
mario-s May 5, 2022
0248157
fmt
mario-s May 5, 2022
e5e171f
revert accidently renaming of file
mario-s May 5, 2022
1a3fe85
tryout proxy
mario-s May 10, 2022
c88c64e
use proxy-tools as forward
mario-s May 10, 2022
01f6fb1
add hint for proxy in README
mario-s May 10, 2022
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
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ tokio = { version = "1.14", features = ["macros", "rt-multi-thread"] }
tracing-log = "0.1"
tracing-subscriber = { version = "0.3", features = ["env-filter"] }
uuid = { version = "1.0", features = ["v4"] }
test-with = "0.7.1"

[features]
default = [
Expand Down
8 changes: 8 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,14 @@ $ TEST_INTEGRATION=1 TEST_DELETE_RECORDS=1 KAFKA_CONNECT=localhost:9094 cargo te
in another session. Note that Apache Kafka supports a different set of features then redpanda, so we pass other
environment variables.

### Using a SOCKS 5 Proxy

To run the integration test via a SOCKS 5 proxy, you need to set the environment variable `SOCKS_PROXY`. The following command requires a running proxy on the local machine.
```console
$ KAFKA_CONNECT=0.0.0.0:9093 SOCKS_PROXY=localhost:8080 cargo test --features full
```
To avoid known issues with the [test library](https://github.com/yanganto/test-with/issues/18), it is recommended to clean up before the first test.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I consider that a pretty major drawback. This is the reason why we use some custom macro for the other environment variables. Would be great if you could use a similar approach.

Copy link
Contributor Author

@mario-s mario-s May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, I changed that to use a macro. However the drawback of this approach is, that you don't see that the test is skipped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, the macro is not ideal. My hope is that rust-lang/rfcs#3221 will be accepted and we can get the best of both worlds. But until then the skipping won't be reported properly.

Copy link

@yanganto yanganto May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi,
this is something you may want to use for this scenario.
https://crates.io/crates/test-with

If you are using CI tool for testing, or under development with source code changing, you do not need cargo clean before testing.

Besides, the test report from cargo will change to the following format for a better understanding.

running 2 tests
test tests::test_ignored ... ignored, because variable not found: NOTHING
test tests::test_works ... ok

test result: ok. 1 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.00s

If there is any issue on the crate, an issue is always welcome. :)

Copy link

Choose a reason for hiding this comment

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

By the way, there is more than one issue in #[ignore(if)], so it might not be the better solution to solve. It is good to keep ignoring in runtime and ignoring under conditions separately.

The following are related PR and issues in Rust, for you to know more about these if you are interested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not to worried about the CI and the env variable handling there. Ignoring tests and build time instead of runtime is IMHO a messy DX (developer experience). Rskafka is already a somewhat complicated crate -- especially when you consider the integration tests. Sure, not having a proper "ignore" status for skipped tests is bad DX as well, but I consider this the lesser evil.

So let's wait until Rust implements the required feature. I've created #139 to track this on our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could assign the #139 to me as well, I would be more than happy to integrate the feature, when Rust is ready.


### Java Interopt
To test if RSKafka can produce/consume records to/from the official Java client, you need to have Java installed and the
`TEST_JAVA_INTEROPT=1` environment variable set.
Expand Down
12 changes: 8 additions & 4 deletions tests/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,19 @@ async fn test_tls() {
.unwrap();
}

// Disabled as currently no SOCKS5 integration tests
#[cfg(feature = "transport-socks5")]
#[ignore]
#[test_with::env(KAFKA_CONNECT, SOCKS_PROXY)]
#[tokio::test]
async fn test_socks5() {
maybe_start_logging();

let client = ClientBuilder::new(vec!["my-cluster-kafka-bootstrap:9092".to_owned()])
.socks5_proxy("localhost:1080".to_owned())
// e.g. "my-cluster-kafka-bootstrap:9092"
let cluster = std::env::var("KAFKA_CONNECT").unwrap();
// e.g. "localhost:1080"
let proxy = std::env::var("SOCKS_PROXY").unwrap();

let client = ClientBuilder::new(vec![cluster])
.socks5_proxy(proxy)
.build()
.await
.unwrap();
Expand Down