Skip to content

Add tests to udp::handlers:handle_announce #91

Merged
josecelano merged 2 commits intotest-udp-handlersfrom
test-handle-announce
Sep 20, 2022
Merged

Add tests to udp::handlers:handle_announce #91
josecelano merged 2 commits intotest-udp-handlersfrom
test-handle-announce

Conversation

@josecelano
Copy link
Copy Markdown
Member

@josecelano josecelano commented Sep 15, 2022

Depends on: #82

@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Sep 15, 2022 •
edited
Loading

There are two "false failure" tests:

I do not know why yet. I do not even know whether the production code is OK and it's a false failure test, or there is a bug.

The errors are:

  • In the announce request using IPV4, I do not get the right peer IP. I get this IP: 0.0.0.0
  • In the announce request using IPV6, I do not get the peer IP at all.

@josecelano josecelano changed the base branch from develop to test-udp-handlers September 15, 2022 17:37
@josecelano josecelano added the Quality & Assurance Relates to QA, Testing, and CI label Sep 15, 2022
@josecelano josecelano changed the title Add tests to udp::handlers:handle_announce function Add tests to udp::handlers:handle_announce Sep 15, 2022
@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Sep 16, 2022

There are two "false failure" tests:

I do not know why yet. I do not even know whether the production code is OK and it's a false failure test, or there is a bug.

The errors are:

  • In the announce request using IPV4, I do not get the right peer IP. I get this IP: 0.0.0.0
  • In the announce request using IPV6, I do not get the peer IP at all.

I found out the problem. Loopback IP addresses have special behaviour, and I was using loopback addresses in the tests; therefore, it's not a bug. I fixed the tests, and I will continue working on adding new tests for the loopback cases.

@josecelano josecelano force-pushed the test-handle-announce branch 7 times, most recently from 9d0a78b to e879b18 Compare September 16, 2022 19:35
@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Sep 16, 2022 •
edited
Loading

hi @da2ce7 @WarmBeer this is almost done.

TODO:

  • Add a couple of tests more. Basically, they are a copy/paste for IPV6 from IPV4 tests.
  • Remove some comments.
  • Try to improve test readability.

The tests are:

udp::handlers::tests::announce_request::from_ipv4::an_announced_peer_should_not_be_included_in_the_response ... ok
test udp::handlers::tests::announce_request::from_ipv4::loopback_ip::the_peer_ip_should_be_changed_to_the_external_ip_in_the_tracker_configuration_if_defined ... ok
test udp::handlers::tests::announce_request::from_ipv4::an_announced_peer_should_be_added_to_the_tracker ... ok
test udp::handlers::tests::announce_request::from_ipv4::the_tracker_should_always_use_the_remote_client_ip_in_the_udp_request_header_instead_of_the_peer_address_in_the_announce_request ... ok
test udp::handlers::tests::announce_request::from_ipv6::an_announced_peer_should_be_added_to_the_tracker ... ok
test udp::handlers::tests::announce_request::from_ipv4::when_the_announce_request_comes_from_a_client_using_ipv4_the_response_should_not_include_peers_using_ipv6 ... ok
test udp::handlers::tests::announce_request::from_ipv6::an_announced_peer_should_not_be_included_in_the_response ... ok
test udp::handlers::tests::announce_request::from_ipv6::when_the_announce_request_comes_from_a_client_using_ipv6_the_response_should_not_include_peers_using_ipv4 ... ok
test

I would like to see some indent on the output for each mod like other testing frameworks, but it seems rust does not support that behaviour. Something like this:

test udp::handlers::tests::announce_request::
	from_ipv4::
		an_announced_peer_should_be_added_to_the_tracker ... ok
		an_announced_peer_should_not_be_included_in_the_response ... ok
		the_tracker_should_always_use_the_remote_client_ip_in_the_udp_request_header_instead_of_the_peer_address_in_the_announce_request ... ok
		when_the_announce_request_comes_from_a_client_using_ipv4_the_response_should_not_include_peers_using_ipv6 ... ok
		loopback_ip::
			the_peer_ip_should_be_changed_to_the_external_ip_in_the_tracker_configuration_if_defined ... ok

test udp::handlers::tests::announce_request::
	from_ipv6::
		an_announced_peer_should_be_added_to_the_tracker ... ok
		an_announced_peer_should_not_be_included_in_the_response ... ok
		when_the_announce_request_comes_from_a_client_using_ipv6_the_response_should_not_include_peers_using_ipv4 ... ok

Ideally lines should be like:

test udp::handlers::tests::an_announce_request::from_ipv4::which_is_a_loopback_ip::shouild_change_the_peer_ip_to_the_external_ip_in_the_tracker_configuration_if_defined ... ok

instead of:

test udp::handlers::tests::announce_request::from_ipv4::loopback_ip::the_peer_ip_should_be_changed_to_the_external_ip_in_the_tracker_configuration_if_defined ... ok

That's why I still must review the names.

I have not changed the prod code except for one new method:

TorrentTracker::tracker.get_all_torrent_peers

It makes tests much more readable. Without that method, I need much more "arrange" code in the test.

We should move a lot of logic out of the controller handle_announce into the ToreentTracker or a new service. In fact, there is duplicate code in the HTTP and UPD controllers (handle_xxx methods). I'm not going to implement those refactors here. This PR is only to add tests.

On the other hand, since the mod is very big now, we could create a new mod handlers with one mod per handler.

@josecelano josecelano marked this pull request as ready for review September 19, 2022 09:45
@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Sep 19, 2022

hi @da2ce7 @WarmBeer, this is ready for review. I think there could be a bug. If you confirm it, we can merge the PR and open a new issue. Remember, I'm only adding tests for the current behaviour in the PR (even if it was wrong).

This was referenced Sep 19, 2022
Merged
Open
@josecelano josecelano merged commit 13adecd into test-udp-handlers Sep 20, 2022
@josecelano josecelano mentioned this pull request Sep 20, 2022
Merged
@josecelano josecelano deleted the test-handle-announce branch January 16, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@da2ce7 da2ce7 Awaiting requested review from da2ce7

@mickvandijke mickvandijke Awaiting requested review from mickvandijke

Assignees

No one assigned

Labels

Quality & Assurance Relates to QA, Testing, and CI

Projects

Torrust Solution
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@josecelano