Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tests #145

Merged
merged 25 commits into from
Nov 2, 2022
Merged

Refactor tests #145

merged 25 commits into from
Nov 2, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 24, 2022

As part of #142, I had a look at the tests and I found them rather hard to understand.

This PR attempts to improve this situation. It is a clean refactoring, no behaviour has been changed and should thus hopefully be non-controversial.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Am I correct assuming that you moved to tests/ to make sure we test the external API of the crate @thomaseizinger?

One comment, otherwise looks good to me.

@@ -56,7 +56,7 @@ fn prop_config_send_recv_single() {
.expect("send_recv")
};

let result = futures::join!(server, client).1;
let result = futures::future::join(server, client).await.1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@@ -131,16 +131,19 @@ impl Stream {
self.id
}

pub fn is_write_closed(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change? If so, this would require a changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new function is not a breaking change I believe!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly me. Sorry for the noise.

@thomaseizinger
Copy link
Contributor Author

Am I correct assuming that you moved to tests/ to make sure we test the external API of the crate @thomaseizinger?

Yes!

@@ -131,16 +131,19 @@ impl Stream {
self.id
}

pub fn is_write_closed(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly me. Sorry for the noise.

@mxinden mxinden merged commit 2fcc567 into libp2p:master Nov 2, 2022
@thomaseizinger thomaseizinger deleted the refactor-tests branch November 2, 2022 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants