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

chore: add tests for NewOverlayFromEthereumAddress method #4485

Merged

Conversation

Aviksaikat
Copy link
Contributor

added 4 test cases under TestNewOverlayFromEthereumAddress to test NewOverlayFromEthereumAddress of crypto library

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

image

@Aviksaikat
Copy link
Contributor Author

@notanatol I have reopened the PR with the suggested updates.

pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
pkg/crypto/crypto_test.go Outdated Show resolved Hide resolved
@notanatol notanatol changed the title chore: add tests in table-test format for NewOverlayFromEthereumAddress chore: add tests for NewOverlayFromEthereumAddress Nov 29, 2023
@notanatol notanatol changed the title chore: add tests for NewOverlayFromEthereumAddress chore: add tests for NewOverlayFromEthereumAddress method Nov 29, 2023
@notanatol
Copy link
Contributor

@notanatol I have reopened the PR with the suggested updates.

thanks, but you don't have to re-open the PR, just push the fix commits to this branch, they are squashed anyway as we merge to master.

@Aviksaikat Aviksaikat requested a review from mrekucci December 2, 2023 00:10
t.Errorf("got address length %v, want %v", l, swarm.HashSize)
}

wantAddress, err := swarm.ParseHexAddress(tc.expectedAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

This step can be avoided by using swarm.MustParseHexAddress function in the test table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only line 303 or from 299?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because MustParseHexAddress is not taking account of the length of the hash

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for the tc.expectedAddress.

pkg/crypto/crypto_test.go Show resolved Hide resolved
pkg/crypto/crypto_test.go Show resolved Hide resolved
pkg/crypto/crypto_test.go Show resolved Hide resolved
@istae istae requested review from acha-bill and notanatol December 12, 2023 13:46
@istae istae merged commit 7c166ba into ethersphere:master Jan 10, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants