Improve CentralDogmaPropertySupplierIntegrationTest

[thachlp]

Related issue: #187
As Javadoc of CentralDogmaRule class we should create rules by

@ClassRule
public static final CentralDogmaRule rule = new CentralDogmaRule();

The problem with @Rule is it will start and stop an embedded server for each test, I think it may affect performance tests and can cause the EndpointSelectionTimeoutException random fail.

[CLAassistant]

All committers have signed the CLA.

[ocadaruma]

Thanks for the patch!

If we share single CentralDogmaRule instance among tests, doesn't it cause potential race between tests? (e.g. testA create file => testB depends on file's absence)

We should avoid bring such dependencies in tests because it makes tests fragile, so we should fix the root cause of EndpointSelectionTimeoutException rather than mitigating the chance by reducing instance creation.

[thachlp]

Thanks for the patch!

If we share single CentralDogmaRule instance among tests, doesn't it cause potential race between tests? (e.g. testA create file => testB depends on file's absence)

We should avoid bring such dependencies in tests because it makes tests fragile, so we should fix the root cause of EndpointSelectionTimeoutException rather than mitigating the chance by reducing instance creation.

@ocadaruma Thanks for replying to me 😊
As I understand, delcaton uses CentralDogma to store properties, so the test should be the same as the use-case behavior, we start only one centraldogma server, and working on this... I also referenced to centraldogma code to see how they test.

From the issue log, I see it is a random fail, so I just think about the problem, we start/stop the dogma server for each test, so we can not sure if the server is already for access or not (even it started success)

[ocadaruma]

H-mm, maybe I still don't quite understand well why this patch is supposed to fix the flaky test.

I understand making CentralDogmaRule as ClassRule can reduce the chance of the failure though, still the root cause is not fixed so same failure could happen, right?

so the test should be the same as the use-case behavior

Current patch could fail depending on the test execution order. (why currently it succeeds is just "lucky").

In testFileExist, it creates a file in the CD project. On the other hand, testFileNonExistent checks the absence of the file.
So if these tests are executed in testFileExist => testFileNonExistent, testFileNonExistent will fail.
(Though JUnit4 determines the test execution order deterministically, we shouldn't write a test that could fail depending on the execution order in general)

So, to get fresh CD instances every time, using @Rule is more simple.

[thachlp]

H-mm, maybe I still don't quite understand well why this patch is supposed to fix the flaky test.

I understand making CentralDogmaRule as ClassRule can reduce the chance of the failure though, still the root cause is not fixed so same failure could happen, right?

so the test should be the same as the use-case behavior

Current patch could fail depending on the test execution order. (why currently it succeeds is just "lucky").

In testFileExist, it creates a file in the CD project. On the other hand, testFileNonExistent checks the absence of the file. So if these tests are executed in testFileExist => testFileNonExistent, testFileNonExistent will fail. (Though JUnit4 determines the test execution order deterministically, we shouldn't write a test that could fail depending on the execution order in general)

So, to get fresh CD instances every time, using @Rule is more simple.

Thank you for clear explanation to me @ocadaruma, so I will close this pr and work on another pr (if I can find the root cause), again, thank you