-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(framework): consolidate and stabilize CredentialsTest #6614
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,36 @@ | ||
| package org.tron.keystore; | ||
|
|
||
| import java.security.NoSuchAlgorithmException; | ||
| import java.security.SecureRandom; | ||
| import junit.framework.TestCase; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
| import org.springframework.util.Assert; | ||
| import org.tron.common.crypto.SignUtils; | ||
| import org.mockito.Mockito; | ||
| import org.tron.common.crypto.SignInterface; | ||
| import org.tron.common.crypto.sm2.SM2; | ||
| import org.tron.common.utils.ByteUtil; | ||
| import org.tron.common.utils.StringUtil; | ||
|
|
||
| @Slf4j | ||
| public class CredentialsTest extends TestCase { | ||
| public class CredentialsTest { | ||
|
|
||
| private static final byte[] ADDRESS_1 = ByteUtil.hexToBytes( | ||
| "410102030405060708090a0b0c0d0e0f1011121314"); | ||
| private static final byte[] ADDRESS_2 = ByteUtil.hexToBytes( | ||
| "411415161718191a1b1c1d1e1f2021222324252627"); | ||
|
|
||
| private SignInterface mockSignInterface(byte[] address) { | ||
| SignInterface signInterface = Mockito.mock(SignInterface.class); | ||
| Mockito.when(signInterface.getAddress()).thenReturn(address); | ||
| return signInterface; | ||
| } | ||
|
|
||
| @Test | ||
| public void testCreate() throws NoSuchAlgorithmException { | ||
| Credentials credentials = Credentials.create(SignUtils.getGeneratedRandomSign( | ||
| SecureRandom.getInstance("NativePRNG"),true)); | ||
| Assert.hasText(credentials.getAddress(),"Credentials address create failed!"); | ||
| Assert.notNull(credentials.getSignInterface(), | ||
| "Credentials cryptoEngine create failed"); | ||
| public void testCreate() { | ||
| SignInterface signInterface = mockSignInterface(ADDRESS_1); | ||
|
|
||
| Credentials credentials = Credentials.create(signInterface); | ||
|
|
||
| Assert.assertEquals("Credentials address create failed!", | ||
| StringUtil.encode58Check(ADDRESS_1), credentials.getAddress()); | ||
| Assert.assertSame("Credentials cryptoEngine create failed", signInterface, | ||
| credentials.getSignInterface()); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -28,21 +39,43 @@ public void testCreateFromSM2() { | |
| Credentials.create(SM2.fromNodeId(ByteUtil.hexToBytes("fffffffffff" | ||
| + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" | ||
| + "fffffffffffffffffffffffffffffffffffffff"))); | ||
| Assert.fail("Expected IllegalArgumentException"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: could we also add a happy-path test for
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lxcmyf Good suggestion. The SM2 happy-path coverage is indeed thin here. However, this PR is focused on stabilizing the existing |
||
| } catch (Exception e) { | ||
| Assert.isInstanceOf(IllegalArgumentException.class, e); | ||
| Assert.assertTrue(e instanceof IllegalArgumentException); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testEquals() throws NoSuchAlgorithmException { | ||
| Credentials credentials1 = Credentials.create(SignUtils.getGeneratedRandomSign( | ||
| SecureRandom.getInstance("NativePRNG"),true)); | ||
| Credentials credentials2 = Credentials.create(SignUtils.getGeneratedRandomSign( | ||
| SecureRandom.getInstance("NativePRNG"),true)); | ||
| Assert.isTrue(!credentials1.equals(credentials2), | ||
| "Credentials instance should be not equal!"); | ||
| Assert.isTrue(!(credentials1.hashCode() == credentials2.hashCode()), | ||
| "Credentials instance hashcode should be not equal!"); | ||
| public void testEquals() { | ||
| Credentials credentials1 = Credentials.create(mockSignInterface(ADDRESS_1)); | ||
| Credentials credentials2 = Credentials.create(mockSignInterface(ADDRESS_2)); | ||
|
|
||
| Assert.assertNotEquals("Credentials address fixtures should differ", | ||
| credentials1.getAddress(), credentials2.getAddress()); | ||
| Assert.assertNotEquals("Credentials instance should be not equal!", | ||
| credentials1, credentials2); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEqualsWithAddressAndCryptoEngine() { | ||
| Object aObject = new Object(); | ||
| SignInterface signInterface = mockSignInterface(ADDRESS_1); | ||
| SignInterface signInterface2 = mockSignInterface(ADDRESS_1); | ||
| SignInterface signInterface3 = mockSignInterface(ADDRESS_2); | ||
|
|
||
| Credentials credential = Credentials.create(signInterface); | ||
| Credentials sameCredential = Credentials.create(signInterface); | ||
| Credentials sameAddressDifferentEngineCredential = Credentials.create(signInterface2); | ||
| Credentials differentCredential = Credentials.create(signInterface3); | ||
|
|
||
| Assert.assertFalse(aObject.equals(credential)); | ||
| Assert.assertFalse(credential.equals(aObject)); | ||
| Assert.assertFalse(credential.equals(null)); | ||
| Assert.assertEquals(credential, sameCredential); | ||
| Assert.assertEquals("Equal credentials must have the same hashCode", | ||
| credential.hashCode(), sameCredential.hashCode()); | ||
| Assert.assertNotEquals(credential, sameAddressDifferentEngineCredential); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One semantic question: this test now locks in that two
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lxcmyf Yes, this is intentional. The existing A This test does not introduce new behavior; it simply documents and verifies the contract already defined in the |
||
| Assert.assertFalse(credential.equals(differentCredential)); | ||
| } | ||
|
|
||
| } | ||
| } | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying! Makes total sense — keeping the new fixture as-is.