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

Use hootbin for tests #703

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Use hootbin for tests #703

merged 1 commit into from
Jan 30, 2024

Conversation

algesten
Copy link
Owner

@algesten algesten commented Jan 20, 2024

This is an attempt to address the problem with using httpbin.org in our tests.

hootbin is a very rough approximation of httpbin built on the library hoot.

hoot is a no_std experimental library where I am exploring how to implement a "correct" HTTP/1.1 client/server using Rust type state variables. I figured as a proof-of-concept I could use it to implement a simple server part for ureq's tests.

If the hoot experiment works out, I might suggest using it to underpin a future ureq 3.0.

Close #688

The biggest drawback of this PR is that it introduces hoot as a main dependency, due to doc tests.

@algesten
Copy link
Owner Author

@jsha unless you have objections, I'd like to merge this.

@algesten
Copy link
Owner Author

Alright. I'll merge this. We can always revert it.

@algesten algesten merged commit c4e0b36 into main Jan 30, 2024
94 checks passed
@algesten algesten deleted the feature/hootbin branch January 30, 2024 19:41
@strideynet
Copy link

This change may need to be reverted - it looks like there's an issue in hoot breaking builds - algesten/hoot#1

@rob-p
Copy link

rob-p commented Jan 31, 2024

I'm running into this as well. It seems it only builds properly on 1.75, it's borked for me on anything at rust 1.74.1 and below wtih:

error[E0445]: crate-private trait `types::Private` in public interface
  --> /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/types.rs:10:1
   |
7  | pub(crate) trait Private {
   | ------------------------ `types::Private` declared as crate-private
...
10 | pub trait State: Private {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-private trait

error[E0445]: crate-private trait `types::Private` in public interface
  --> /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/types.rs:12:1
   |
7  | pub(crate) trait Private {
   | ------------------------ `types::Private` declared as crate-private
...
12 | pub trait Version: Private {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-private trait

error[E0445]: crate-private trait `types::Private` in public interface
  --> /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/types.rs:19:1
   |
7  | pub(crate) trait Private {
   | ------------------------ `types::Private` declared as crate-private
...
19 | pub trait Method: Private {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-private trait

error[E0445]: crate-private trait `types::Private` in public interface
  --> /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/types.rs:42:1
   |
7  | pub(crate) trait Private {
   | ------------------------ `types::Private` declared as crate-private
...
42 | pub trait BodyType: Private {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-private trait

error[E0446]: crate-private type `Writer<'_, '_>` in public interface
  --> /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/header.rs:60:1
   |
60 | / pub fn check_and_output_header(
61 | |     mut w: Writer,
62 | |     version: HttpVersion,
63 | |     name: &str,
64 | |     bytes: &[u8],
65 | |     trailer: bool,
66 | | ) -> Result<()> {
   | |_______________^ can't leak crate-private type
   |
  ::: /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/out.rs:42:1
   |
42 |   pub(crate) struct Writer<'b, 'a> {
   |   -------------------------------- `Writer<'_, '_>` declared as crate-private

error[E0446]: private type `CallState` in public interface
   --> /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/body.rs:10:1
    |
10  | / pub fn do_read_body<'a, 'b>(
11  | |     state: &mut CallState,
12  | |     src: &'a [u8],
13  | |     dst: &'b mut [u8],
14  | | ) -> Result<BodyPart<'b>> {
    | |_________________________^ can't leak private type
    |
   ::: /home/redacted/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hoot-0.1.2/src/lib.rs:106:1
    |
106 |   pub(crate) struct CallState {
    |   --------------------------- `CallState` declared as private

@tnull
Copy link

tnull commented Jan 31, 2024

This change also breaks builds with any rustc older than 1.65, which is very unfortunate: #713

@algesten
Copy link
Owner Author

Thanks! I'm solving this now.

@tnull
Copy link

tnull commented Jan 31, 2024

Thanks! I'm solving this now.

Thanks for the quick reaction, and especially also for the introduction of a 1.61 MSRV. It's great to have that guarantee!

However, I'm a bit dubious if doctests really warrant the introduction of hoot to the dependency tree of all users running ureq in production? After all it's an entirely experimental library with a single author (you) and there are many projects that don't like having such dependencies in their tree for all kinds of reasons, most prominently security considerations.

@algesten
Copy link
Owner Author

I know. When I started that journey I didn't realise doc tests require non-dev deps. The reliance on httpbin.org for all things was not working anymore – making new ureq releases to address rustls, cookie etc version had just ground to a halt.

The choice was between making hoot/hootbin a full dependency or starting from scratch with solving the impasse. I don't have the time for that now, and I figured it's more important to get other dependencies and fixes up-to-date.

@edmorley
Copy link
Contributor

edmorley commented Feb 2, 2024

However, I'm a bit dubious if doctests really warrant the introduction of hoot to the dependency tree of all users running ureq in production?

I also ended up here after seeing an unexpected extra dependency appear in Cargo.lock in a Dependabot PR update. One of the reasons I chose ureq was because of it having much fewer dependencies than say reqwest, so if there was a way to avoid this extra test-only dep I'd appreciate it :-)

Some ideas (apologies if you've already tried these and ruled them out):

  1. Add hoot as an optional = true dep (but don't add it to default-features, and instead activate it only here), and add suitable feature guards to agent() etc (perhaps behind a feature name of test?)
  2. Or, if (1) isn't viable because otherwise running a plain cargo test locally wouldn't work (and you'd have to run cargo test --all-features or cargo test --features test or similar locally each time), then you could still do (1) but add the test feature to default-features and users like myself could at least consume ureq with default-features = false

@algesten
Copy link
Owner Author

algesten commented Feb 2, 2024

Hi @edmorley,

I don't mind having optional features for running the tests. I'll explore this weekend.

@algesten
Copy link
Owner Author

Didn't work btw. :(

@edmorley
Copy link
Contributor

Didn't work btw. :(

I presume this was before you figured it out - I see #729 has now made hootbin an optional dependency. Thank you for fixing this! :-)

@algesten
Copy link
Owner Author

Yeah. I found the solution minutes after I posted :)

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.

Replace httpbin.org in all examples and tests
5 participants