refactor custom metadata integration test #82714
Conversation
currently, custom metadata integration test combines 3-4 test cases in a single test method. This change split the test into separate scenarios and removes code duplication.
idegtiarenko
added
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (Team:Distributed) |
server/src/internalClusterTest/java/org/elasticsearch/snapshots/CustomMetadataSnapshotIT.java
Outdated
Show resolved
Hide resolved
| var metadata = clusterAdmin().prepareState().get().getState().getMetadata(); | ||
| logger.info("check that api custom metadata [{}] is visible via api", metadata); | ||
| assertThat(metadata.<ApiMetadata>custom(ApiMetadata.TYPE).getData(), equalTo("before_restart_s_gw")); | ||
| assertThat(metadata.<NonApiMetadata>custom(NonApiMetadata.TYPE), nullValue()); |
There was a problem hiding this comment.
Since this checks also gateway and api may be the test class should be renamed to CustomMetadataScopeIT or CustomMetadataXContextIT?
There was a problem hiding this comment.
Just a stupid question, why do we need the <NonApiMetadata> here now?
Not sure about the renaming, maybe CustomMetadataContextIT would be more appropriate to point so we cover the word Context. But I'm indifferent tbh. as far as I'm concerned the current name is good enough too :)
There was a problem hiding this comment.
Just a stupid question, why do we need the here now?
This is needed to avoid explicit cast that we had here before:
assertThat(((SnapshottableMetadata) metadata.custom(SnapshottableMetadata.TYPE)).getData(), equalTo("before_snapshot_s"));
Not sure about the renaming, maybe CustomMetadataContextIT would be more appropriate to point so we cover the word Context. But I'm indifferent tbh. as far as I'm concerned the current name is good enough too :)
Then I would rename to CustomMetadataContextIT as half of the test cases here are not related to the snapshots.
original-brownbear
left a comment
original-brownbear
left a comment
There was a problem hiding this comment.
LGTM nice cleanup!
| var metadata = clusterAdmin().prepareState().get().getState().getMetadata(); | ||
| logger.info("check that api custom metadata [{}] is visible via api", metadata); | ||
| assertThat(metadata.<ApiMetadata>custom(ApiMetadata.TYPE).getData(), equalTo("before_restart_s_gw")); | ||
| assertThat(metadata.<NonApiMetadata>custom(NonApiMetadata.TYPE), nullValue()); |
There was a problem hiding this comment.
Just a stupid question, why do we need the <NonApiMetadata> here now?
Not sure about the renaming, maybe CustomMetadataContextIT would be more appropriate to point so we cover the word Context. But I'm indifferent tbh. as far as I'm concerned the current name is good enough too :)
server/src/internalClusterTest/java/org/elasticsearch/snapshots/CustomMetadataSnapshotIT.java
Outdated
Show resolved
Hide resolved
# Conflicts: # server/src/internalClusterTest/java/org/elasticsearch/snapshots/CustomMetadataSnapshotIT.java
3 participants
currently, custom metadata integration test combines 3-4 test cases in
a single test method. This change split the test into separate scenarios
and removes code duplication.