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.
| Assert.assertEquals(credential, sameCredential); | ||
| Assert.assertEquals("Equal credentials must have the same hashCode", | ||
| credential.hashCode(), sameCredential.hashCode()); | ||
| Assert.assertNotEquals(credential, sameAddressDifferentEngineCredential); |
There was a problem hiding this comment.
One semantic question: this test now locks in that two Credentials objects with the same address but different SignInterface instances are not equal. Is that the intended long-term contract?
There was a problem hiding this comment.
@lxcmyf Yes, this is intentional. The existing Credentials.equals() implementation already includes both cryptoEngine and address in its equality check, and hashCode() is consistent with this behavior.
A Credentials object represents a specific signing identity. Even if two instances share the same address, a different SignInterface (i.e., cryptoEngine) implies a different signing capability, so they should not be considered equal.
This test does not introduce new behavior; it simply documents and verifies the contract already defined in the public boolean equals(Object o) method of Credentials.java.
| Credentials.create(SM2.fromNodeId(ByteUtil.hexToBytes("fffffffffff" | ||
| + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" | ||
| + "fffffffffffffffffffffffffffffffffffffff"))); | ||
| Assert.fail("Expected IllegalArgumentException"); |
There was a problem hiding this comment.
Non-blocking: could we also add a happy-path test for Credentials.create(SM2)? Right now the SM2 overload is only covered through the invalid-input case.
There was a problem hiding this comment.
@lxcmyf Good suggestion. The SM2 happy-path coverage is indeed thin here. However, this PR is focused on stabilizing the existing CredentialsTest fixtures. Adding new SM2 coverage would require additional crypto setup, so it’s better handled in a separate follow-up PR. I’ll track it for future work.
6 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.