[
https://issues.apache.org/jira/browse/IGNITE-22388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Mikhail Efremov updated IGNITE-22388:
-------------------------------------
Description:
*Description*
During IGNITE-22315 we found that tests
{{TableManagerTable#tableManagerStopTest*}} are failed and after investigations
there was clear that several static mocks aren't in use in tests that leads to
actually unworked tests. The special case was fixed in IGNITE-22355, but there
are more issued places, especially described below. The main goal of the ticket
is to revise the test class and rewrite it in accordings to its purpose without
any excessive, unused code lines.
*Motivation*
There will be provided 3 examples with problematic code the was found in
{{TableManagerTable#tableManagerStopTest*}} but not limited.
The first code smell is the name of the tests:
{{{}TableManagerTable#tableManagerStopTest1-4{}}}. The names doesn't tell how
1st and 4th are different.
The second is another (except that in IGNITE-22355) static mock
{{schemaServiceMock}} in try-with-resources block that shouldn't work outside
the block:
{code:java}
private TableViewInternal mockManagersAndCreateTableWithDelay(
String tableName,
CompletableFuture<TableManager> tblManagerFut,
@Nullable Phaser phaser
) throws Exception {
String consistentId = "node0";
// ...
when(ts.getByConsistentId(any())).thenReturn(new ClusterNodeImpl(
UUID.randomUUID().toString(),
consistentId,
new NetworkAddress("localhost", 47500)
));
try (MockedStatic<SchemaUtils> schemaServiceMock =
mockStatic(SchemaUtils.class)) {
schemaServiceMock.when(() -> SchemaUtils.prepareSchemaDescriptor(any()))
.thenReturn(mock(SchemaDescriptor.class));
}
try (MockedStatic<AffinityUtils> affinityServiceMock =
mockStatic(AffinityUtils.class)) {
ArrayList<List<ClusterNode>> assignment = new ArrayList<>(PARTITIONS);
for (int part = 0; part < PARTITIONS; part++) {
assignment.add(new ArrayList<>(Collections.singleton(node)));
}
affinityServiceMock.when(() ->
AffinityUtils.calculateAssignments(any(), anyInt(), anyInt()))
.thenReturn(assignment);
}
//...
{code}
In the test class there are only two such mocks and {{affinityServiceMock}} is
removed in IGNITE-22355, but {{schemaServiceMock}} still requires reworking.
The third issue in the code above: the problem is that {{ts}} isn't really
uses: {{ts}} is topology service field in the test class, but for
{{TableManager}} creation topology service arrives from a mocked cluster
service:
{code:java}
private TableManager createTableManager(/*...*/) {
TableManager tableManager = new TableManager(
// ...
clusterService.topologyService(),
//...
{code}
{{topologyService}} is mocked in {{before}} with another mock:
{code:java}
@BeforeEach
void before() throws NodeStoppingException {
// ...
TopologyService topologyService = mock(TopologyService.class);
when(clusterService.topologyService()).thenReturn(topologyService);
// ...
{code}
That means, that {{ts}} field isn't in use and the code above is just for
nothing.
There only 3 arguments, but they shows drastical problems with the test class
and the ticket calls for the class reworking.
*Definition of done*
# Tests' names should have meaningful names that differs one from another.
# Tests shouldn't contain unused and meaningless code.
was:
*Description*
During IGNITE-22315 we found that tests
{{TableManagerTable#tableManagerStopTest*}} are failed and after investigations
there was clear that several static mocks aren't in use in tests that leads to
actually unworked tests. The special case was fixed in IGNITE-22355, but there
are more issued places, especially described below. The main goal of the ticket
is to revise the test class and rewrite it in accordings to its purpose without
any excessive, unused code lines.
*Motivation*
There will be provided 3 examples with problematic code the was found in
{{TableManagerTable#tableManagerStopTest*}} but not limited.
The first code smell is the name of the tests:
{{TableManagerTable#tableManagerStopTest1-4}}. The names doesn't tell how 1st
and 4th are different.
The second is another (except that in IGNITE-22355) static mock
{{schemaServiceMock}} in try-with-resources block that shouldn't work outside
the block:
{code:title=|language=java|collapse=false}private TableViewInternal
mockManagersAndCreateTableWithDelay(
String tableName,
CompletableFuture<TableManager> tblManagerFut,
@Nullable Phaser phaser
) throws Exception {
String consistentId = "node0";
when(ts.getByConsistentId(any())).thenReturn(new ClusterNodeImpl(
UUID.randomUUID().toString(),
consistentId,
new NetworkAddress("localhost", 47500)
));
try (MockedStatic<SchemaUtils> schemaServiceMock =
mockStatic(SchemaUtils.class)) {
schemaServiceMock.when(() -> SchemaUtils.prepareSchemaDescriptor(any()))
.thenReturn(mock(SchemaDescriptor.class));
}
try (MockedStatic<AffinityUtils> affinityServiceMock =
mockStatic(AffinityUtils.class)) {
ArrayList<List<ClusterNode>> assignment = new ArrayList<>(PARTITIONS);
for (int part = 0; part < PARTITIONS; part++) {
assignment.add(new ArrayList<>(Collections.singleton(node)));
}
affinityServiceMock.when(() ->
AffinityUtils.calculateAssignments(any(), anyInt(), anyInt()))
.thenReturn(assignment);
}
//...
{code}
In the test class there are only two such mocks and {{affinityServiceMock}} is
removed in IGNITE-22355, but {{schemaServiceMock}} still requires reworking.
The third issue in the code above is {{String consistentId = "node0"}} for mock
{{when(ts.getByConsistentId(any()))}}. The problem is that {{ts}} isn't really
uses: {{ts}} is topology service field in the test class, but for
{{TableManager}} creation topology service arrives from a mocked cluster
service:
{code:title=|language=java|collapse=false}private TableManager
createTableManager(/*...*/) {
TableManager tableManager = new TableManager(
// ...
clusterService.topologyService(),
//...
{code}
{{topologyService}} is mocked in {{before}} with another mock:
{code:title=|language=java|collapse=false}@BeforeEach
void before() throws NodeStoppingException {
// ...
TopologyService topologyService = mock(TopologyService.class);
when(clusterService.topologyService()).thenReturn(topologyService);
// ...
{code}
That means, that {{ts}} field isn't in use and the code above is just for
nothing.
There only 3 arguments, but they shows drastical problems with the test class
and the ticket calls for the class reworking.
*Definition of done*
# Tests' names should have meaningful names that differs one from another.
# Tests shouldn't contain unused and meaningless code.
> TableManagerTest revision
> -------------------------
>
> Key: IGNITE-22388
> URL: https://issues.apache.org/jira/browse/IGNITE-22388
> Project: Ignite
> Issue Type: Improvement
> Reporter: Mikhail Efremov
> Priority: Major
> Labels: ignite-3
>
> *Description*
> During IGNITE-22315 we found that tests
> {{TableManagerTable#tableManagerStopTest*}} are failed and after
> investigations there was clear that several static mocks aren't in use in
> tests that leads to actually unworked tests. The special case was fixed in
> IGNITE-22355, but there are more issued places, especially described below.
> The main goal of the ticket is to revise the test class and rewrite it in
> accordings to its purpose without any excessive, unused code lines.
> *Motivation*
> There will be provided 3 examples with problematic code the was found in
> {{TableManagerTable#tableManagerStopTest*}} but not limited.
> The first code smell is the name of the tests:
> {{{}TableManagerTable#tableManagerStopTest1-4{}}}. The names doesn't tell how
> 1st and 4th are different.
> The second is another (except that in IGNITE-22355) static mock
> {{schemaServiceMock}} in try-with-resources block that shouldn't work outside
> the block:
> {code:java}
> private TableViewInternal mockManagersAndCreateTableWithDelay(
> String tableName,
> CompletableFuture<TableManager> tblManagerFut,
> @Nullable Phaser phaser
> ) throws Exception {
> String consistentId = "node0";
> // ...
> when(ts.getByConsistentId(any())).thenReturn(new ClusterNodeImpl(
> UUID.randomUUID().toString(),
> consistentId,
> new NetworkAddress("localhost", 47500)
> ));
> try (MockedStatic<SchemaUtils> schemaServiceMock =
> mockStatic(SchemaUtils.class)) {
> schemaServiceMock.when(() ->
> SchemaUtils.prepareSchemaDescriptor(any()))
> .thenReturn(mock(SchemaDescriptor.class));
> }
> try (MockedStatic<AffinityUtils> affinityServiceMock =
> mockStatic(AffinityUtils.class)) {
> ArrayList<List<ClusterNode>> assignment = new ArrayList<>(PARTITIONS);
> for (int part = 0; part < PARTITIONS; part++) {
> assignment.add(new ArrayList<>(Collections.singleton(node)));
> }
> affinityServiceMock.when(() ->
> AffinityUtils.calculateAssignments(any(), anyInt(), anyInt()))
> .thenReturn(assignment);
> }
> //...
> {code}
> In the test class there are only two such mocks and {{affinityServiceMock}}
> is removed in IGNITE-22355, but {{schemaServiceMock}} still requires
> reworking.
> The third issue in the code above: the problem is that {{ts}} isn't really
> uses: {{ts}} is topology service field in the test class, but for
> {{TableManager}} creation topology service arrives from a mocked cluster
> service:
> {code:java}
> private TableManager createTableManager(/*...*/) {
> TableManager tableManager = new TableManager(
> // ...
> clusterService.topologyService(),
> //...
> {code}
> {{topologyService}} is mocked in {{before}} with another mock:
> {code:java}
> @BeforeEach
> void before() throws NodeStoppingException {
> // ...
> TopologyService topologyService = mock(TopologyService.class);
> when(clusterService.topologyService()).thenReturn(topologyService);
> // ...
> {code}
> That means, that {{ts}} field isn't in use and the code above is just for
> nothing.
> There only 3 arguments, but they shows drastical problems with the test class
> and the ticket calls for the class reworking.
> *Definition of done*
> # Tests' names should have meaningful names that differs one from another.
> # Tests shouldn't contain unused and meaningless code.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
