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.

Sign up for GitHub

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

Jump to bottom

test(other): using testify properly #1433

Merged
merged 26 commits into from
Jul 25, 2024
Merged

test(other): using testify properly #1433

merged 26 commits into from
Jul 25, 2024

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Jul 24, 2024 •
edited
Loading

Description

In some cases, we used the expected and actual argument of Eqaul's assertion interchangeably. I changed them and now we will have more clear error messages when we have failed tests.

Also, we had cases to test panics but we used a defer and recover(), I've changed them to assert.Panics.

In gRPC we had cases where we were using t.Error and ==, I've just used assertion there...

Note 1: utils/linkedmap is not refactored in this PR since it has a huge amount of tests with this issue, I thought it would take a lot of time and energy, and at the same time the chance of changing it and its tests it very low!

Note 2: there is no linter to check whether a Golang code uses a testing package to Testify properly! maybe writing one? we have several Golang code bases on pactus using Testify...

Related issue

@kehiy kehiy requested review from b00f and Ja7ad July 24, 2024 22:12
@kehiy kehiy changed the title refactor(tests): using testify properly refactor: using testify properly Jul 24, 2024
@kehiy kehiy changed the title refactor: using testify properly refactor(other): using testify properly Jul 24, 2024
@b00f
Copy link
Collaborator

b00f commented Jul 25, 2024 •
edited
Loading

@kehiy The PR commit type should be test(other). Not refactor.

test 		Adding missing tests or correcting existing tests
refactor 	A code change that neither fixes a bug nor adds a feature

@kehiy kehiy changed the title refactor(other): using testify properly test(other): using testify properly Jul 25, 2024
@kehiy
Copy link
Contributor Author

kehiy commented Jul 25, 2024

@b00f Done.

maxipaz
maxipaz approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@maxipaz maxipaz left a comment

Choose a reason for hiding this comment

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

LGTM! Just left some minor comments 🙂

config/config_test.go Outdated Show resolved Hide resolved
sortition/vrf_test.go Outdated Show resolved Hide resolved
www/grpc/server_test.go Show resolved Hide resolved
@kehiy kehiy requested a review from maxipaz July 25, 2024 12:29
@kehiy
Copy link
Contributor Author

kehiy commented Jul 25, 2024

@maxipaz Requested changes fixed. please review.

@Ja7ad Ja7ad added this to the v1.4.0 milestone Jul 25, 2024
@kehiy kehiy merged commit 5057337 into pactus-project:main Jul 25, 2024
10 checks passed
@kehiy kehiy deleted the tests branch July 25, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxipaz maxipaz maxipaz approved these changes

@b00f b00f b00f approved these changes

@Ja7ad Ja7ad Awaiting requested review from Ja7ad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.4.0
Development

Successfully merging this pull request may close these issues.

Defining tests properly
4 participants
@kehiy @b00f @maxipaz @Ja7ad