Skip to content

Refactor proposal for test asserts#172

Closed
josecelano wants to merge 1 commit intotorrust:developfrom
josecelano:refactor-tests-asserts
Closed

Refactor proposal for test asserts#172
josecelano wants to merge 1 commit intotorrust:developfrom
josecelano:refactor-tests-asserts

Conversation

@josecelano
Copy link
Copy Markdown
Member

@josecelano josecelano commented Feb 2, 2023 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

da2ce7 commented Feb 4, 2023

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

@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Feb 4, 2023

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 josecelano mentioned this pull request Feb 6, 2023
Closed
@josecelano
Copy link
Copy Markdown
Member Author

josecelano commented Feb 6, 2023

@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
Copy link
Copy Markdown
Member Author

josecelano commented Aug 3, 2023

Moving this to a discussion: #356

@josecelano josecelano closed this Aug 3, 2023
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

None yet

Projects

Torrust Solution
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@josecelano @da2ce7