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

Replace httpmock with hyper #190

Merged
merged 27 commits into from
Nov 26, 2021
Merged

Replace httpmock with hyper #190

merged 27 commits into from
Nov 26, 2021

Conversation

ducaale
Copy link
Owner

@ducaale ducaale commented Nov 11, 2021

Also introduces a feature flag named online-tests that can be used to filter out tests that require an internet connection.

Resolves #186

@ducaale ducaale changed the title Remove httpmock Replace httpmock with hyper Nov 11, 2021
Comment on lines +18 to +19
// The panic_rx channel is to make sure the test fails if the server panics.
// If the server panics, the message is not sent and the recv call panics.
Copy link
Owner Author

@ducaale ducaale Nov 12, 2021

Choose a reason for hiding this comment

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

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 in that server (and can't tell which server it is) so I can't view that message

Copy link
Owner Author

@ducaale ducaale Nov 17, 2021

Choose a reason for hiding this comment

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

It is in tokio's discord server which has channels for hyper, h3, reqwest and more.

@ducaale ducaale requested a review from blyxxyz November 12, 2021 10:41
@blyxxyz
Copy link
Collaborator

blyxxyz commented Nov 15, 2021

The problem with x86_64-unknown-linux-gnu seems to be that Ubuntu 20.04 by default disables the ciphers TLS1.1 needs to function, even if you enable that TLS version. HTTPie also suffers from this but it has a --ciphers flag that can presumably help. IIRC reqwest and native-tls don't let you set the cipher suite.

An easy fix is to enable the vendored feature on the openssl crate. That's not something we'd want to do by default but it could make sense to offer outside of testing.

An even easier fix is to change the good_tls_version_nativetls test to test TLS1.2 instead. I think that's the best option for now.

See #190 (comment)
for why the test was failing on x86_64-unknown-linux-gnu before this
change
@blyxxyz
Copy link
Collaborator

blyxxyz commented Nov 15, 2021

I managed to get the integration tests working on ARM:

/// Cargo-cross for ARM runs tests using qemu.
///
/// It sets an environment variable like this:
/// CARGO_TARGET_ARM_UNKNOWN_LINUX_GNUEABIHF_RUNNER=qemu-arm
fn find_runner() -> Option<String> {
    for (key, value) in std::env::vars() {
        if key.starts_with("CARGO_TARGET_") && key.ends_with("_RUNNER") && !value.is_empty() {
            return Some(value);
        }
    }
    None
}

fn get_base_command() -> Command {
    let mut cmd;
    let path = assert_cmd::cargo::cargo_bin("xh");
    if let Some(runner) = find_runner() {
        cmd = Command::new(runner);
        cmd.arg(path);
    } else {
        cmd = Command::new(path);
    }
    cmd.env("HOME", "");
    #[cfg(target_os = "windows")]
    cmd.env("XH_TEST_MODE_WIN_HOME_DIR", "");
    cmd
}

tests/cli.rs Outdated Show resolved Hide resolved
@ducaale
Copy link
Owner Author

ducaale commented Nov 15, 2021

Woohoo! I can't believe that integration tests are finally working for ARM. How does the solution work? are there any docs I can refer to?

Oh, and should we get rid of the integration-tests feature flag?

@blyxxyz
Copy link
Collaborator

blyxxyz commented Nov 15, 2021

I don't know if it's documented anywhere (and how safely we can rely on it).

Cross uses qemu to run binaries for other architectures. You prefix a command with e.g. qemu-arm and then qemu translates it for your host system.

Cross fiddles with cargo to make sure your test binaries are run through qemu. But we build our own commands and they aren't fixed automatically.

I had a suspicion it was something like this, and discovered the details and this solution by panicking in the tests with the output of pgrep -af . and env.

I think this would've worked before cross-rs/cross#122, when it still used binfmt and didn't need to make the runner explicit.

Oh, and should we get rid of the integration-tests feature flag?

Yeah, I think we no longer have a need for it.

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

This looks great! I also like the online-tests feature.

I noticed some inconsistency between writing http::Response and hyper::Response, use http::Response; could clean things up. But that doesn't really matter.

Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +18 to +19
// The panic_rx channel is to make sure the test fails if the server panics.
// If the server panics, the message is not sent and the recv call panics.
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 in that server (and can't tell which server it is) so I can't view that message

@blyxxyz
Copy link
Collaborator

blyxxyz commented Nov 26, 2021

I'm ok with merging now and re-enabling the test after chromium/badssl.com#482 is fixed.

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.

2 participants