http-server-rs / http-server

Simple and configurable command-line HTTP server
https://crates.io/crates/http-server
Apache License 2.0
176 stars 20 forks source link

fix: broken tests from legacy versions #402

Closed Antosser closed 7 months ago

Antosser commented 9 months ago

Fixed 4 of the 7 failed tests. All of them have to do with paths, which work differently depending on the OS. I still have no idea how to fix the other 2.

cargo test:

failures:

---- addon::compression::gzip::tests::compresses_body stdout ----
thread 'addon::compression::gzip::tests::compresses_body' panicked at src\addon\compression\gzip.rs:220:9:
assertion `left == right` failed
  left: 6552
 right: 6364
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- config::file::tests::parses_config_from_file stdout ----
thread 'config::file::tests::parses_config_from_file' panicked at src\config\file.rs:84:60:
called `Result::unwrap()` on an `Err` value: Failed to parse config from file. TOML parse error at line 5, column 24
  |
5 |             root_dir = "./fixtures"
  |                        ^^^^^^^^^^^^
invalid type: string "./fixtures", expected a borrowed string

---- utils::url_encode::tests::encodes_uri stdout ----
thread 'utils::url_encode::tests::encodes_uri' panicked at src\utils\url_encode.rs:49:9:
assertion `left == right` failed
  left: "/%5C/these%20are%20important%20files/do_not_delete/file%20name.txt"
 right: "/these%20are%20important%20files/do_not_delete/file%20name.txt"

failures:
    addon::compression::gzip::tests::compresses_body
    config::file::tests::parses_config_from_file
    utils::url_encode::tests::encodes_uri

test result: FAILED. 47 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

error: test failed, to rerun pass `--lib`

I still don't understand why integration tests don't get run

EstebanBorai commented 9 months ago

@Antosser will update CI workflow so I can test this against that.

EstebanBorai commented 9 months ago

@Antosser please provide a moro descriptive PR title. I added a workflow to run tests with cargo test

https://github.com/http-server-rs/http-server/blob/92eb0aee73db6432461c523db6700e9819da7df0/.github/workflows/test.yml#L32-L33

You just have to uncomment when rebasing! Thanks in advance!

Antosser commented 9 months ago

Why use bats? Have never heard of it. Wouldn't it be easier to write the tests in Rust? @EstebanBorai

EstebanBorai commented 9 months ago

Why use bats? Have never heard of it. Wouldn't it be easier to write the tests in Rust? @EstebanBorai

Bats allows us to test commands using the CLI just like the user will do.

For integration/unit tests we continue to use Rust.

Tests that you fixed, for instance, will remain.

Antosser commented 9 months ago

@EstebanBorai It only works for UNIX though. And I still don't understand: Why do the integration tests not get run when I do cargo test?

EstebanBorai commented 9 months ago

@EstebanBorai It only works for UNIX though. And I still don't understand: Why do the integration tests not get run when I do cargo test?

Hi @Antosser!

I don't see tests running in this CI session, have you tried rebasing?

Antosser commented 9 months ago

@EstebanBorai It only works for UNIX though. And I still don't understand: Why do the integration tests not get run when I do cargo test?

Hi @Antosser!

I don't see tests running in this CI session, have you tried rebasing?

I don't mean here on GitHub. I mean when I run them locally

EstebanBorai commented 9 months ago

Hey @Antosser! I did some rearrangement on integration tests here: https://github.com/http-server-rs/http-server/pull/404.

Those tests should run programmatically, but they depend on having a HTTP Server running with certain configuration.

So I was thinking about spawning a process for them and kill the process after running the test. I will see if theres something better or fallback to that if Im not able to find a solution soon.

WRT windows compatibility, have you ever used WSL2? Nowadays is perhaps the de-facto dev setup on Windows.

EstebanBorai commented 9 months ago

Hey @Antosser! I did some rearrangement on integration tests here: #404.

Those tests should run programmatically, but they depend on having a HTTP Server running with certain configuration.

So I was thinking about spawning a process for them and kill the process after running the test. I will see if theres something better or fallback to that if Im not able to find a solution soon.

WRT windows compatibility, have you ever used WSL2? Nowadays is perhaps the de-facto dev setup on Windows.

Thinking deeper, these tests on /test actually don't make sense now that we have bats.

Antosser commented 9 months ago

Hey @Antosser! I did some rearrangement on integration tests here: #404.

Those tests should run programmatically, but they depend on having a HTTP Server running with certain configuration.

So I was thinking about spawning a process for them and kill the process after running the test. I will see if theres something better or fallback to that if Im not able to find a solution soon.

WRT windows compatibility, have you ever used WSL2? Nowadays is perhaps the de-facto dev setup on Windows.

Rust can do all that

EstebanBorai commented 9 months ago

Hey @Antosser! I did some rearrangement on integration tests here: #404.

Those tests should run programmatically, but they depend on having a HTTP Server running with certain configuration.

So I was thinking about spawning a process for them and kill the process after running the test. I will see if theres something better or fallback to that if Im not able to find a solution soon.

WRT windows compatibility, have you ever used WSL2? Nowadays is perhaps the de-facto dev setup on Windows.

Rust can do all that

Thats right. Actually every programming language can do it.

But its more about the approach and the resources.

With Bats its not required to recompile on test changes.

Another good point is that we want to mimic user experience, using bash we are using the exact same approach as the user.

And bash is very powerful and easy to write.

Antosser commented 9 months ago

I guess these are tests that only don't pass on Windows, because the paths are different

Tho, it's still important to fix since a portion of users will still run this on Windows

Antosser commented 8 months ago

Should I close since we've switched to bats?

Antosser commented 7 months ago

I really want to develop this app further

EstebanBorai commented 7 months ago

I really want to develop this app further

Hey @Antosser! It would be nice to have CLI tests somehow, feel free to share ideas in a GitHub Discussion here: https://github.com/http-server-rs/http-server/discussions/413