IGNITE-11708 "Unable to run tests in IgniteConfigVariationsAbstractTest subclasses" #6434
Conversation
| @@ -108,9 +92,17 @@ protected static void injectTestsConfiguration(VariationsTestsConfig testsCfgInj | |||
| return false; | |||
| } | |||
|
|
|||
| /** Check that test name is not null. */ | |||
| @Before | |||
| public void checkTestName(){ | |||
There was a problem hiding this comment.
What is the purpose of that check? Is it specific and necessary for IgniteConfigVariationsAbstractTest?
There was a problem hiding this comment.
I left it from the previous code. Such check already exists in GridAbstractTest [1].
During run IgniteConfigVariationsAbstractTest subclasses, I did not catch this assertion. But maybe it exists a specific case in which check is necessary.
[1] https://github.com/apache/ignite/blob/master/modules/core/src/test/java/org/apache/ignite/testframework/junits/GridAbstractTest.java#L188
| @@ -108,9 +92,17 @@ protected static void injectTestsConfiguration(VariationsTestsConfig testsCfgInj | |||
| return false; | |||
| } | |||
|
|
|||
| /** Check that test name is not null. */ | |||
| @Before | |||
| public void checkTestName(){ | |||
There was a problem hiding this comment.
Also space is missing before {.
| assert testsCfg != null; | ||
| assert testsCfgInjected != null; | ||
|
|
||
| testsCfg = testsCfgInjected; |
There was a problem hiding this comment.
Now testsCfg is assigned before all tests in a class. Before that change it was assigned before each test method execution. Could you please explain why it still works as expected?
There was a problem hiding this comment.
as I see testsCfg do not change during test methods execution. So the reason for me was to remove unnecessary initialization.
But I can try to move such initialization to beforeTest() method in IgniteConfigVariationsAbstractTest class to leave the same logic as it was before.
There was a problem hiding this comment.
I mean that we inject TestConfigurations only once [1] and I do not see a reason to re-initialize testCfg before each method.
[1] https://github.com/apache/ignite/blob/master/modules/core/src/test/java/org/apache/ignite/testframework/test/ConfigVariationsTestSuiteBuilderTest.java#L401
There was a problem hiding this comment.
@1vanan I suppose it is expected to work as follows:
- Test classes are created using a code generation library.
testsCfgInjected(static member) is assigned on@BeforeClasslevel.testsCfg(instance member) is assigned on@Beforelevel.
A test class instance is created for each test method execution. So, I do not understand how an expected configuration is injected to each test instance. Could you please that configurations are injected as expected? I suspect that dummyCfg is injected everywhere now.
There was a problem hiding this comment.
@pavlukhin you are definitely right.
Please check the last commit - I inserted [1] debug info for checking testCfg. When we assign testsCfgInjected before test class, it returns "Dummy config", before each test - another cfg description.
So I think to remain the last version (in the last commit) - in beforeTest method.
[1] https://github.com/apache/ignite/pull/6434/files#diff-78ef107e580c29ced70d7eb0739f2c7bR147
|
|
||
| /** Executes after test class. Checks that {@link #testCheck()} method was executed indeed. */ | ||
| @AfterClass | ||
| public static void validatetestExecution(){ |
|
|
||
| /** JUnit test method. */ | ||
| @Test | ||
| public void testCheck(){ |
| @@ -60,12 +61,12 @@ | |||
|
|
|||
| /** */ | |||
| @RunWith(IgniteContinuousQueryConfigVariationsSuite.SuiteSingleNode.class) | |||
| public static class SingleNodeTest { | |||
| @Ignore("https://issues.apache.org/jira/browse/IGNITE-11851") public static class SingleNodeTest { | |||
There was a problem hiding this comment.
I suppose @Ignore should be placed on a separate line according to our conventions.
| } | ||
|
|
||
| /** */ | ||
| @RunWith(IgniteContinuousQueryConfigVariationsSuite.SuiteMultiNode.class) | ||
| public static class MultiNodeTest { | ||
| @Ignore("https://issues.apache.org/jira/browse/IGNITE-11851") public static class MultiNodeTest { |
There was a problem hiding this comment.
I suppose @Ignore should be placed on a separate line.
| @@ -144,6 +144,7 @@ public void testKeyAffinityDeploy() throws Exception { | |||
| */ | |||
| @Test | |||
| public void testMultipleDeploy() throws Exception { | |||
| System.out.println("~~! " + testsCfg.description()); | |||
There was a problem hiding this comment.
I suggest to use GridAbstractTest.info here (if this output is really needed).
There was a problem hiding this comment.
@pavlukhin No, it is just to debug in the context of testsCfg question.
I'll remove it in the next commit.
| @@ -92,22 +73,17 @@ | |||
| @Override public String getTestIgniteInstanceName() { | |||
| return "testGrid"; | |||
| } | |||
|
|
|||
| /** | |||
| * Invoked by reflection from {@code ConfigVariationsTestSuiteBuilder}. | |||
There was a problem hiding this comment.
I think that we can add a comment to testsCfg fields stating that it is assigned by generated code. Otherwise it is not clear how and where is it assigned making it hard to understand by future maintainers.
There was a problem hiding this comment.
Ok, I paid attention to this moment in comments
No description provided.