Skip to content

HDDS-13868. Add unit test coverage for OMNodeDetails#9245

Merged
jojochuang merged 5 commits intoapache:masterfrom
0lai0:HDDS-13868
Nov 5, 2025
Merged

HDDS-13868. Add unit test coverage for OMNodeDetails#9245
jojochuang merged 5 commits intoapache:masterfrom
0lai0:HDDS-13868

Conversation

@0lai0
Copy link
Contributor

@0lai0 0lai0 commented Nov 4, 2025

What changes were proposed in this pull request?

This PR adds unit tests for the OMNodeDetails class.
Added test file: TestOMNodeDetails.java with 16 test methods covering major functionality.

Please describe your PR in detail:

Test Coverage

  1. Builder pattern:
  • testBuilderWithInetSocketAddress() — building from InetSocketAddress
  • testBuilderWithHostAddressString() — building from host address string
  • testSetRatisAddress() — setting Ratis address in builder
  1. State management:
  • testRatisListenerFlag() — Ratis listener flag (get, set, builder)
  • testDecommissionedState() — decommissioned state transitions
  1. Accessor methods:
  • testGetRpcPort() — RPC port retrieval
  • testToString() — string representation
  • testGetOMPrintInfo() — formatted print info
  1. Protobuf serialization:
  • testProtobufConversionActiveNode() — active node conversion
  • testProtobufConversionDecommissionedNode() — decommissioned node conversion
  • testProtobufConversionListenerNode() — listener node conversion
  1. Configuration loading:
  • testGetOMNodeAddressFromConf() — reading OM node address from config
  • testGetOMNodeDetailsFromConfValid() — loading node details from valid config
  • testGetOMNodeDetailsFromConfMissing() — handling missing configuration
  1. URL generation:
  • testGetOMDBCheckpointEndpointUrlHttp() — HTTP checkpoint endpoint URL
  • testGetOMDBCheckpointEndpointUrlHttps() — HTTPS checkpoint endpoint URL

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13868

How was this patch tested?

mvn test -Dtest=TestOMNodeDetails -pl hadoop-ozone/common

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Looks good. Just one comment and we're good to go.

assertEquals(nodeId, nodeDetails.getNodeId());
assertEquals(9862, nodeDetails.getRpcPort());
assertEquals(9873, nodeDetails.getRatisPort());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add two negative tests here

Suggested change
}
nodeDetails = OMNodeDetails.getOMNodeDetailsFromConf(conf, serviceId, null);
assertNull(nodeDetails);
nodeDetails = OMNodeDetails.getOMNodeDetailsFromConf(conf, null, nodeId);
assertNull(nodeDetails);
}

Copy link
Contributor Author

@0lai0 0lai0 Nov 5, 2025

Choose a reason for hiding this comment

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

@jojochuang Thank you for your suggestion. I think we could put this two negative test to testGetOMNodeDetailsFormConfMissing(). Is this a good way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

@jojochuang jojochuang merged commit bc577ae into apache:master Nov 5, 2025
55 of 56 checks passed
Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

+1 merging it

@0lai0
Copy link
Contributor Author

0lai0 commented Nov 6, 2025

Thank you @jojochuang .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants