test(framework): consolidate and stabilize CredentialsTest#6614
test(framework): consolidate and stabilize CredentialsTest#66143for wants to merge 2 commits intotronprotocol:developfrom
Conversation
Consolidate the misplaced keystroe CredentialsTest into org.tron.keystore.CredentialsTest. - remove the duplicate test under the misspelled keystroe package - add explicit equals behavior coverage for address and cryptoEngine - normalize assertions to JUnit Assert and remove legacy TestCase usage
Replace random Credentials test setup with deterministic SignInterface mocks so the suite no longer depends on platform-specific SecureRandom providers or probabilistic retries. - remove NativePRNG usage from CredentialsTest - replace random key generation with fixed address fixtures via mocked SignInterface - assert create(SignInterface) returns the expected base58check address - keep equals/hashCode contract coverage with deterministic inputs
| private static final byte[] ADDRESS_1 = ByteUtil.hexToBytes( | ||
| "410102030405060708090a0b0c0d0e0f1011121314"); |
There was a problem hiding this comment.
Nice work on the fixture improvement! Just a quick question out of curiosity — the old "TQhZ7...".getBytes() returns 34 UTF-8 bytes rather than a proper 21-byte address, so switching to ByteUtil.hexToBytes("410102...") definitely improves semantic correctness. That said, does this change have any practical impact on the test outcomes, or is it more of a correctness/readability improvement on the fixture side? Just want to make sure I'm not missing anything.
There was a problem hiding this comment.
@CodeNinjaEvan Yes, this is mainly a fixture correctness improvement rather than an intended behavioral change.
SignInterface.getAddress() is expected to return raw address bytes, so using a canonical 21-byte TRON address fixture is a better fit for the API contract. The previous Base58 string UTF-8 bytes could still make the test pass because Credentials.create(...) currently just feeds the returned bytes into encode58Check(...) without validating the address shape.
So it would be better to keep the new fixture as-is. It makes the test semantically correct, and it doesn’t materially change the current behavior under test.
There was a problem hiding this comment.
@CodeNinjaEvan Yes, this is mainly a fixture correctness improvement rather than an intended behavioral change.
SignInterface.getAddress()is expected to return raw address bytes, so using a canonical 21-byte TRON address fixture is a better fit for the API contract. The previous Base58 string UTF-8 bytes could still make the test pass becauseCredentials.create(...)currently just feeds the returned bytes intoencode58Check(...)without validating the address shape.So it would be better to keep the new fixture as-is. It makes the test semantically correct, and it doesn’t materially change the current behavior under test.
Thanks for clarifying! Makes total sense — keeping the new fixture as-is.
4 participants
What does this PR do?
This PR cleans up and consolidates
CredentialsTestcoverage in theframeworkmodule.org.tron.keystroeorg.tron.keystoreSignInterfacefixturesCredentials.create(...),equals(...), andhashCode()Why are these changes required?
The previous test setup had two problems:
This change makes the test deterministic, easier to understand, and better aligned with the current
Credentialscontract:addressis derived fromgetAddress()cryptoEngineandaddresshashCodeThis PR has been tested by:
./gradlew framework:test --tests org.tron.keystore.CredentialsTestFollow up
No follow-up is required.
Extra details
This is a test-only change. No production logic, protocol behavior, or runtime code path is modified.