Notification tests

[deefdragon]

This is a PR of tests for the notifications. The SMTP notification tests first included only test the expected route, and do not cover any of the custom subject code.

This is mostly to confirm that this format is valid for usage with the rest of the notifications, but when approved, both erroring routes, and custom subjects will be tested.

[louislam]

It is amazing! Thanks for the big work.

However, I am still concerning that, because the tests only checked whether it is called correctly and it is not actually call to the API. Is it really enough to ensure everything is correct?

It is because unexpected situations would come from the other ends.

[deefdragon]

There were/still will be some required manual tests for major changes to the notifications to make sure that everything is proper, but that's really unavoidable, and I think the automated tests here are, if nothing else, a really good way to double check work (I have ~half a dozen small fixes and 2-3 TODOs already in the code from the first 5 notifiers). To go much further would be testing the services themselves anyway, which is out of scope of automated tests.

TLDR: Yes I think its enough if combined with the (already done IIRC) manual validation.

As a side note: Outside of SMTP, and apprise, its "create an object, and pass it to some transport". Both apprise and smtp are only barely exceptions, apprise using the message and URLs separately, and smtp having to build the transport internally.

It feels like there should be a way to make everything more flexible for future messages, but I am still thinking on it.

[louislam]

There were/still will be some required manual tests for major changes to the notifications to make sure that everything is proper

So that is what I am concerning, because eventually I am not able to test all of them manually. Because:

• Some of them are paid services such as SMS.
• Some of them are from specific countries like AliyunSMS and DingDing, I cannot even register an account.

And why I accept these pull requests in the first place if I can't test it, because they shown me the screenshots.

In other words, #751 should not apply all notification providers in this stage, just because of testing problems.

It should only apply to providers which I am able to test it manually.

If you understand what I mean, I could mark them in the classes.

Like:

class Discord extends NotificationProvider {
    name = "discord"; 

    // true or false
    allowTemplate = true;

    ...
}

======

And sorry about that, usually I won't be that harsh. However, notification is a core feature of Uptime Kuma definitely. Most users are depending on it, so stable is always the highest priority in this part. If I don't take this seriously, it will be another 'statping' which just buggy and don't send any notification.

Also unpredictable bugs are always unpredictable. In the past, for example, #436. Although I tested working on many platforms, once it released, some users reported that their containers were not able to start after updated.

[deefdragon]

I will say that by large changes, I mean addition of a new notification/the notification's api changes, but I get the feelings expressed.

I think a lot of this problem boils down to the fact that a lot of the work on the notifications side is custom per notification.

If it was possible to just trust the notifications to send the message, then any future changes would just need to be done to server/notification.js, leaving most of the implementations alone. Right now, many notifications are creating custom objects based on the heartbeat/monitor.

-- brainstorming --
The simplest solution would be to limit to just sending raw strings, and move message parsing to server/notification.js, but this would mean that a number features such as the cards in slack/discord etc. would be removed, and email subjects would not be possible.

A complicated potential option,( and yes I am bringing this back to templates, apologies for the one track mind RN) might be something like this

class Template {
    //IsObject is used to tell the template parser if the template should be interpreted as a
    //full object, or as a string.
    IsObject: boolean = false;
    //The resulting data from the
    Data: string | Object = "";
}

/* what happens if object vs string vs null for a template.
"message": 
    if string: the default simple message sent to the notification provider.
    if object: This becomes the base object/json that is used instead of the pre-constructed one.
    if null: throw error
"subject": 
    if string: the subject used in the email
    if object: converted to a string to then use
    if null: Uses message as subject
etc...

*/
//the actual send function in the notification.
function send(notification: any, data: Map<string, Template>): string {
    //"build" the object(s) using the templates.
    //run any template specific post processing like signing, or setting of api keys etc.
    //send to the destination.
    //return success/failure
}

Unfortunately, to reach the same level of features, you need to develop some complex templates for discord etc, which just moves the problem back to the template construction.

It should be easier to test the templates in this form mind, as the inputs and outputs from notification.send will be much more predictable & comparable to the default, but its not perfect, and it would take time to implement.

This is a problem that likely needs fixing, regardless of the solution chosen (if its not possible to test the notifications for future changes, then it becomes much more difficult to add other notifications like for certificates), but trying to fix it almost creates more problems.

[deefdragon]

Assuming these all pass, this is ready to be reviewed & merged.

[tarun7singh]

@deefdragon Working on adding tests for the newly added click send SMS notification integration.
Do I need to create a new branch and PR for it?

[deefdragon]

@tarun7singh yeah, new PR is needed. Mind, @louislam needs to merge this one in first.

[louislam]

Wondering it is able to put all *.spec.js in the test directory?

[deefdragon]

Wondering it is able to put all *.spec.js in the test directory?

I can move the tests if you wish, but I would advise against that kind of layout. Having all tests in one directory/a mirrored test directory tree can lead to a very messy/difficult to upkeep structure, whereas having the tests next to the code they test makes it a lot easier to work on them. (No need for ../../../src/server/foo for example. Just ./foo)

I don't know if/how Vue auto-generates tests, but with angular, when components are generated, the tests are generated at the same time, in the same folder. Go uses a similar layout.

[deefdragon]

Going to close due to legacy.