test: add oap-server core alarm package test case

[jsbxyyx]

Please answer these questions before submitting pull request

• Why submit this pull request?

• Bug fix

• New feature provided

• Improve performance

• Related issues
  Increase the test coverage rate #2438

---

Bug fix

• Bug description.

• How to fix?

---

New feature or improvement

• Describe the details and related test reports.

[kezhenxu94]

Hi @jsbxyyx , thanks for your contribution.

It's not about "right" or "wrong", but there're already many good practices in unit testing, like mocking and verifying, and it seems that you're testing from scratch, causing the test codes too verbose to read, take the AlarmEntranceTest as example, here is a version that I think might be more straightforward, and short, using some existed testing framework that is already included in this project:

public class AlarmEntranceTest {
    @Test
    public void forwardVerifyDoNotInitMethod() {
        ModuleDefineHolder holder = mock(ModuleDefineHolder.class);
        when(holder.has(AlarmModule.NAME)).thenReturn(false);

        AlarmEntrance alarmEntrance = new AlarmEntrance(holder);
        alarmEntrance.forward(mock(Metrics.class));

        Object o = Whitebox.getInternalState(alarmEntrance, "metricsNotify");
        Assert.assertNull(o);
    }

    @Test
    public void forwardVerifyDoInitMethod() {
        MetricsNotify mockMetricsNotify = mock(MetricsNotify.class);

        ModuleDefineHolder holder = mock(ModuleDefineHolder.class, RETURNS_DEEP_STUBS);
        when(holder.has(AlarmModule.NAME)).thenReturn(true);
        when(holder.find(AlarmModule.NAME).provider().getService(MetricsNotify.class)).thenReturn(mockMetricsNotify);

        AlarmEntrance alarmEntrance = new AlarmEntrance(holder);
        alarmEntrance.forward(mock(Metrics.class));
        verify(mockMetricsNotify).notify(any());

        assertNotNull(Whitebox.getInternalState(alarmEntrance, "metricsNotify"));
    }
}

Same idea applies to other test classes in this PR, like AlarmStandardPersistenceTest.

Again ,it's not about "right" or "wrong", but following your style, the test codes may explode rapidly, and hard to maintain

[jsbxyyx]

@kezhenxu94 I thought we were using junit4，if using junit5 then the test case will a lot simple.

[kezhenxu94]

@kezhenxu94 I thought we were using junit4，if using junit5 then the test case will a lot simple.

Even we're using JUnit4, the codes should not necessarily be that complicated and verbose, do we have plan to migrate to JUnit5 @wu-sheng

[jsbxyyx]

@kezhenxu94 I was wrong, if using mockito the test case will a lot simple

[wu-sheng]

I have no plan for JUnit5. What do you expect from it?

[wu-sheng]

I think I need to highlight the test principle. Every test case will cost our CI resources, which are limited. So, you need to make sure the tests are meaningful.

We are not chasing the high coverage, we hope our tests are for

1. Avoiding bug.
2. Show usages and purposes of the target method.

If the method can't be covered, so be it. Covering every line doesn't mean the codes are good.

[wu-sheng]

One thing needs to add, CI with IT tests, we more recommend real Integration tests to make API using in real cases. Many structure codes are not needed to test, such as provider, module definition and module config class.

One core target, making core better, don't chase the number.

[wu-sheng]

For test interests, try to make coverage of receiver(s) better, mock the core module, and give the receiver some input, verify the source flowing into the core. Those are very meaningful tests for your time.

[jsbxyyx]

I agree

[jsbxyyx]

Which modules have higher priority for use cases

[wu-sheng]

Trace receiver

[kezhenxu94]

The method names is not as what we discussed in #3552 (comment)

I'm ok with this PR @wu-sheng , considering that @jsbxyyx you will contribute more test, please pay special attention to what we've discussed, :) thanks

[wu-sheng]

I don't support and object to this PR, because, it covers methods, but at the same time, don't verify the real logic. This costs the CI CPU and time, but can't tell why should test these methods.

@jsbxyyx In the alarm case, I suggest you read my alarm test case. Such as an easy one, AlarmMessageFormatterTest. It tests the format logic going well. If it breaks, we know we are facing issue about alarm test format. That is what we mean about logic tests.

From my standpoint, there should be tests focusing on AlarmNotifyWorker. And verify in a mock MetricsNotify. Could you get my point? The alarm in core module is the bridge, so it could take the responsibilities as a bridge, and ship data as expected, that is the test goal.

dismiss

[wu-sheng]

Don't set test case to the method one by one. I know some books tell so, but I and many SkyWalking PMC don't believe that clearly @kezhenxu94 thinks so too. We are doing white box tests for workflow.

[jsbxyyx]

@wu-sheng @kezhenxu94 OK, I understand.

[jsbxyyx]

The method names is not as what we discussed in #3552 (comment)

I'm ok with this PR @wu-sheng , considering that @jsbxyyx you will contribute more test, please pay special attention to what we've discussed, :) thanks

Before, the conclusion is do not use _ ? style is it like this methodNameDoesWhatWhenTheseConditions?

[kezhenxu94]

The method names is not as what we discussed in #3552 (comment)
I'm ok with this PR @wu-sheng , considering that @jsbxyyx you will contribute more test, please pay special attention to what we've discussed, :) thanks

Before, the conclusion is do not use _ ? style is it like this methodNameDoesWhatWhenTheseConditions?

Not using _ is mandatory, as for method names, I'm ok as long as it can tell the purpose of that test case, the most important point is as what @wu-sheng said above

[kezhenxu94]

Please open new PR to do the tests, thanks

[jsbxyyx]

thanks