Refactor proposal for test asserts

[josecelano]

hi @da2ce7 @WarmBeer, I'm not happy with the asserts I've written so far for the integration tests.

For example. Considering this test:

#[tokio::test]
async fn should_return_the_list_of_previously_announced_peers() {
    let http_tracker_server = start_public_http_tracker().await;

    let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap();

    // Peer 1
    let previously_announced_peer = PeerBuilder::default()
        .with_peer_id(&peer::Id(*b"-qB00000000000000001"))
        .build();

    // Add the Peer 1
    http_tracker_server.add_torrent(&info_hash, &previously_announced_peer).await;

    // Announce the new Peer 2. This new peer is non included on the response peer list
    let response = Client::new(http_tracker_server.get_connection_info())
        .announce(
            &QueryBuilder::default()
                .with_info_hash(&info_hash)
                .with_peer_id(&peer::Id(*b"-qB00000000000000002"))
                .query(),
        )
        .await;

    // It should only contain teh previously announced peer
    assert_announce_response(
        response,
        &Announce {
            complete: 2,
            incomplete: 0,
            interval: http_tracker_server.tracker.config.announce_interval,
            min_interval: http_tracker_server.tracker.config.min_announce_interval,
            peers: vec![DictionaryPeer::from(previously_announced_peer)],
        },
    )

when it fails, it gives you a not-very-good message.

When the response body is wrong:

test http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers ... FAILED

failures:

---- http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers stdout ----
thread 'http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers' panicked at 'assertion failed: `(left == right)`
  left: `Announce { complete: 2, incomplete: 0, interval: 120, min_interval: 120, peers: [DictionaryPeer { ip: "126.0.0.1", peer_id: "2d71423030303030303030303030303030303031", port: 8080 }] }`,
 right: `Announce { complete: 3, incomplete: 0, interval: 120, min_interval: 120, peers: [DictionaryPeer { ip: "126.0.0.1", peer_id: "2d71423030303030303030303030303030303031", port: 8080 }] }`', tests/http/asserts.rs:18:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
           

When the HTTP status is wrong:

running 1 test
test http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers ... FAILED

failures:

---- http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers stdout ----
thread 'http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers' panicked at 'assertion failed: `(left == right)`
  left: `200`,
 right: `404`', tests/http/asserts.rs:14:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 48 filtered out; finished in 0.02s

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

The problem is it gives you the line number in the assert.

I think there are two solutions:

1. We use macros for asserts.
2. We introduce a new struct with the data we want to check.

This PR is a proof of concept for the second solution. Of course, we could use the standard From trait to instantiate this new struct from the response instead of the explicit constructor.

This new implementations shows this output:

running 1 test
test http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers ... FAILED

failures:

---- http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers stdout ----
thread 'http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers' panicked at 'assertion failed: `(left == right)`
  left: `HttpResponse { status: 200, announce: Announce { complete: 2, incomplete: 0, interval: 120, min_interval: 120, peers: [DictionaryPeer { ip: "126.0.0.1", peer_id: "2d71423030303030303030303030303030303031", port: 8080 }] } }`,
 right: `HttpResponse { status: 502, announce: Announce { complete: 2, incomplete: 0, interval: 120, min_interval: 120, peers: [DictionaryPeer { ip: "126.0.0.1", peer_id: "2d71423030303030303030303030303030303031", port: 8080 }] } }`', tests/http_tracker.rs:365:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    http_tracker_server::for_all_config_modes::receiving_an_announce_request::should_return_the_list_of_previously_announced_peers

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 48 filtered out; finished in 0.02s

What do you think?

[da2ce7]

I think that using the From trait would be the standard way of achieving this type of conversion.

[josecelano]

I think that using the From trait would be the standard way of achieving this type of conversion.

Hey, yes. It was only a fast example to support my main question. If you think it worths refactoring the asserts I will create a new issue. It's something not so important, I think.

[josecelano]

@da2ce7 has used an alternative solution here:

pub async fn assert_ok(response: Response) {
    let response_status = response.status().clone();
    let response_headers = response.headers().get("content-type").cloned().unwrap();
    let response_text = response.text().await.unwrap();

    let details = format!(
        r#"
   status: ´{response_status}´
  headers: ´{response_headers:?}´
     text: ´"{response_text}"´"#
    );

    assert_eq!(response_status, 200, "details:{details}.");
    assert_eq!(response_headers, "application/json", "\ndetails:{details}.");
    assert_eq!(response_text, "{\"status\":\"ok\"}", "\ndetails:{details}.");
}

With this formatted string, we can eliminate the need for a new struct for this grouped data.

pub struct ResponseDetails {
    pub status: String,
    pub headers: String,
    pub text: String,
}

but when the higher-level assert fails, I would like to show the original line where it fails, not one of these internal asserts. The good thing about this solution is we show the whole context regardless of which assertion failed.

[josecelano]

Moving this to a discussion: #356