Refactor UdpClient to return errors instead of panicking #814
Conversation
There was a problem hiding this comment.
Hi @ngthhu thank you!! Look good. However, some workflows are failing. You can start fixing:
You can run at least some commands locally before committing. For example:
Clippy
CARGO_INCREMENTAL=0 cargo clippy --no-deps --tests --benches --examples --workspace --all-targets --all-features -- -D clippy::correctness -D clippy::suspicious -D clippy::complexity -D clippy::perf -D clippy::style -D clippy::pedanticUnit tests
cargo testI would suggest installing Clippy in your IDE.
You can run all commands in the testing workflow locally.
What I do is:
- Use Visual Studio Code with Clippy (and other extensions)
- Run unit tests after small changes
cargo test. - Run E2E tests before committing (
cargo run --bin e2e_tests_runner ./share/default/config/tracker.e2e.container.sqlite3.toml).
Let me know if you have any trouble fixing those errors.
Regarding the new feature to test some trackers in bulk. I would do it after merging all console commands in one. See #660
Finally, I would split this PR in two anyway:
- One for changing the API returning an error instead of
panicking. - Another to add the new feature to test a list of trackers.
http_trackers.sh
Outdated
There was a problem hiding this comment.
Hi @ngthhu, I've just started the PR review, I'm commenting on this and other things in the next minutes.
First, we favor scripting in Rust over Bash unless Bash offers crucial features or simplifies script maintenance or implementation. Se, for example:
https://github.com/torrust/torrust-tracker/blob/develop/src/bin/e2e_tests_runner.rs
The reasons are:
- It's enjoyable
- It's easier to test
- Developers can also contribute, not only sys admins. I mean, it lowers the entry barrier to maintain those scripts (in my opinion).
On the other hand, this looks like a good feature for the UDP client. It could accept both:
- A filepath to a file containing a list of trackers (trackers.txt)
- It could be passed via stdin.
cargo run --bin udp_tracker_client announce --trackers="./tracker.txt"
9c38422213e30bff212b30c360d26f9a02136422
cat ./tracker.txt cargo run --bin udp_tracker_client announce 9c38422213e30bff212b30c360d26f9a02136422I have yet to think about the best option to pass that argument. I would depend on what it's the standard or best way to do it using clap. I will think about this and give more feedback.
trackers.txt
Outdated
There was a problem hiding this comment.
@ngthhu I would move this file to a fixtures folder (maybe tests/fixtures/) like this:
https://github.com/torrust/torrust-index/tree/develop/tests/fixtures/torrents
And I would rename it to http_trackers.txt.
Maybe we don't need to add the list to the repo. Maybe we could get an updated list from: https://newtrackon.com/. For example, for HTTP trackers:
https://newtrackon.com/api/http
We could run the test like this:
curl https://newtrackon.com/api/http | cargo run --bin http_tracker_client announce 9c38422213e30bff212b30c360d26f9a02136422or
curl -o http_trackers.txt https://newtrackon.com/api/http
cargo run --bin http_tracker_client announce --tracker="./http_trackers.txt" 9c38422213e30bff212b30c360d26f9a02136422
|
Thank you for your responses @josecelano.
Currently I'm using Visual Studio Code too and I see Clippy running everytime I save a
I haven't done this before. When I run I will run E2E test later then provide more info.
I did some test on another issue and forgot to exclude them in this pull request. I manually edited the list so it contain only http url without the |
OK. Sometimes, you could have errors in workflows (but not locally) just because they started using a newer Rust version. I usually work with the updated nightly version.
You will need to setup some system dependencies like docker and other things. Let me know if docs are not clear enougth.
Cool! It's a nice-to-have feature. I wanted to do it just to see if the client works fine with different trackers. But I'm focused on other things now. |
I run E2E test on my wsl2 with docker and there's a permission error I followed this solution, then restart docker, wsl distro and that error is fixed. |
Hi, permissions are a very common problem with docker, unfortunately :-/. Sometimes, what I do (if I don't want to run E2E tests locally because they are very slow) is open a draft PR with just one commit while I'm working on the next commit (task increment). I check the workflow after a while to see if workflows have passed, if there is an error I fix it locally. This way, I can take advantage of both my PC and the runners :-). But sometimes, it is also slow because we are using shared runners. Besides, if you find a problem in the future, I would suggest immediately opening an issue while you are working on fixing it. There are two advantages:
|
They are very really slow, thank you for your method. |
We run an isolated tracker for each test to avoid coupling between tests and because some tests need a custom tracker configuration. Running tests only takes 14 seconds. The slowest part is the docker build with Rust compilation. In this project, the integration tests are more important. E2E tests are just to check containerization. In the Index they are more important because we test a lot of behaviour that could not be tested without a running tracker (at that time). In both the Tracker and the Index I added a lot of E2E and Integration tests because there were no tests and I had to do a lot of reactorings like web server migration. It was the only way to cover the main functionality. Now, I would like to progressively create more unit tests and fewer integration/E2E tests. It should be easier. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #814 +/- ##
===========================================
+ Coverage 78.91% 78.92% +0.01%
===========================================
Files 163 163
Lines 9186 9212 +26
===========================================
+ Hits 7249 7271 +22
- Misses 1937 1941 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @ngthhu, Thank you!
In the future, you can return a Result in a test function:
https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html#tests-and-
I think that would make the tests more concise by removing the "matches". I don't know if it's possible in this particular case.
|
ACK effca56 |
|
Hi @josecelano, I think this pull request would resolve issue #671 as well. |
Hi, @ngthhu, I've replied here. |
I replaced panics with error handling using
anyhow::Result<T>, which is equivalent tostd::result::Result<T, anyhow::Error>. Although I can runcargo runwithout errors, I'm not certain if there are any hidden bugs.