Improve test suite by starting electron only when needed

[rejas]

Speaking of: the test suite could be so much improved. Currently for every test we start electron, while in most cases we could just "virtually" start the module and test what getDom returns.

Originally posted by @MichMich in #2401 (comment)

[MichMich]

And while we're at it. It might be worth rethinking the test suite we are using. I'm not a specialist on (javascript) testing. But the output of our current test is rather underwhelming. I like the output of Jest. But maybe there is a better alternative.

[fewieden]

I had to work with multiple test frameworks for javascript in my company such as mocha, ava, jest, ...

My favorite test framework of all (the ones I used) is definitely Jest.

I used it for the last few days to test one of my modules from 0 to 100% coverage MMM-soccer. Since this worked quite nicely I intend to apply this also to my other modules.

The tests are mainly unit tests with a few integration tests. I didn't create any e2e tests like the ones we have for the default modules with electron. Instead, I created simple mocks for NodeHelper and Module, to avoid cloning magic mirror in the CI like in this example.

My suggestion would be to cover most logic as unit and integration test, which doesn't require to spin up electron, but using mocks where needed. In addition, electron can be used for specific tests like rendering or related to electron itself.

Maybe @roramirez can add some thoughts to it, as I think he created the initial setup.

[MichMich]

This sounds so much better than what we currently have. It would also simplify building test, especially if we create some test tools users can use.

[roramirez]

Hi guys!

The main idea to create the e2e was try the cover all most cases of the principal modules and find and get it the bug in configuration and start of the app in Electron. At in this time (4 year ago) the tests for new feature/bugfix are 0 and the way most easily to cover from the module to the display in the html render for Electron was build a e2e test.

The unit test are good but we try get and test what the user can see in the Mirror. The problem here the issue is for run a heavy load and many tests in the e2e in CI. Issue like dependencies between run/start/stop of app in middle of every test case have a lot of cost of resources and sometime the stop or start not run the best way.

Maybe the use of Jest can help to build new tests or replaces the current ones. But the replace the all tests suite can be a huge battle of work

[khassel]

I did some tests with electron v12 and spectron v14 and 27 e2e tests are failing ...

Spectron is more or less unmaintained, see this post.

So I think it makes no sense to investigate in this. We should stay on electron v11 until we have a new test setup for e2e with Jest or whatever.

[rejas]

Given the lack of responses to that Spectron post, switching to another test setup seems like a good idea in the long run. Have you had any experience with jest or whatever yet @khassel ?

[fewieden]

@rejas jest is more of a replacement for mocha, since it is a testing framework.

The question is if spectron/electron is really necessary for the e2e tests, where we check the rendered results. We probably can also do snapshot testing of the result from the nunjuck rendering https://github.com/MichMich/MagicMirror/blob/491f5aa7769a12de7f20f201de0078704c158a8d/js/module.js#L84-L110 then we only need to mock the document createElement function

[khassel]

Meanwhile I managed to move more tests using electron to use server only. Will provide a PR after #2664 is merged because its based on this (see this branch).

Because this new tests are using jsdom and therefore running in the browser all test configs used must be changed because require cannot be used anymore.

There are 2 module tests still using electron (calendar and weather). They are complex (using basic-auth or mock-data), will look into this in a following PR.

The other 2 remaining electron tests env_spec and dev_console could be removed without a replacement (there is a new env_spec under e2e with most functionality).

The question is, if we want still some electron tests or if we want to remove the spectron stuff completely.

[rejas]

Great to hear about your progress, good job!

The question is, if we want still some electron tests or if we want to remove the spectron stuff completely.

Would that mean we cannot test if MM runs in Electron without problems?

[khassel]

Would that mean we cannot test if MM runs in Electron without problems?

if we decide to have no electron tests, yes.

[rejas]

Making good progress, awesome!

Would that mean we cannot test if MM runs in Electron without problems?

if we decide to have no electron tests, yes.

I guess basic electron fucntionality like starting and such should be covered by tests. What dou you think @MichMich ?

[MichMich]

Yeah I think the aim is to test everything that's possible without the need of booting electron. And then have one suite of tests covering basic electron functionality.

[khassel]

Agree testing electron functionality.

Problem is spectron, last version is for electron v13. Meanwhile we have electron v14 and v15 released but there is still no spectron version supporting them.

What do you think of the following idea. We start mm as electron app with npm run start & and then use node-fetch to test it, e.g.:

const fetch = require("node-fetch");

describe("App environment", function () {

        it("get request from http://localhost:8080 should return 200", function (done) {
                fetch("http://localhost:8080")
                .then((res) => {
                        expect(res.status).toBe(200);
                        return res.text();
                })
                .then((res) => {
                        expect(res).toContain("<title>MagicMirror  </title>");
                        done();
                });
        });

        it("get request from http://localhost:8080/nothing should return 404", function (done) {
                fetch("http://localhost:8080/nothing").then((res) => {
                        expect(res.status).toBe(404);
                        done();
                });
        });

});

Tested this already and it works (beside the problem to stop mm after the test).

If we use this approach we could remove spectron and migrate to newer electron versions.

[MichMich]

But that wouldn't test the functionality of electron, does it? It only tests the server side of MagicMirror². Which we can just test using the server only feature.

It might be valuable to test the launching of electron as well. And especially if we can test if electron correctly receives all user definable settings.

[khassel]

With npm run start I think mm is started in "electron" mode:

node@00f7b5752dc7:/opt/magic_mirror$ ps a
  PID TTY      STAT   TIME COMMAND
    1 pts/0    Ss     0:00 /usr/bin/tini -- ./entrypoint.sh /opt/setup.sh
    8 pts/0    S      0:00 /bin/bash /opt/setup.sh
   39 pts/0    S      0:00 /bin/bash
  121 pts/0    Sl     0:14 Xvfb :99 -screen 0 1024x768x16
 2169 pts/0    Sl     0:00 npm run start
 2180 pts/0    S      0:00 sh -c DISPLAY="${DISPLAY:=:0}" ./node_modules/.bin/electron js/electron.js
 2181 pts/0    Sl     0:00 node ./node_modules/.bin/electron js/electron.js
 2188 pts/0    Sl     0:00 /opt/magic_mirror/node_modules/electron/dist/electron js/electron.js
 2190 pts/0    S      0:00 /opt/magic_mirror/node_modules/electron/dist/electron --type=zygote --no-zygote-sandbox --no-sandbox
 2191 pts/0    S      0:00 /opt/magic_mirror/node_modules/electron/dist/electron --type=zygote --no-sandbox
 2219 pts/0    Sl     0:00 /opt/magic_mirror/node_modules/electron/dist/electron --type=utility --utility-sub-type=network.mojom.NetworkService --field-trial-handle=104344059122076
 2223 pts/0    Sl     0:00 /opt/magic_mirror/node_modules/electron/dist/electron --type=renderer --no-sandbox --autoplay-policy=no-user-gesture-required --field-trial-handle=104344
 2251 pts/0    Sl     0:00 /opt/magic_mirror/node_modules/electron/dist/electron --type=gpu-process --field-trial-handle=1043440591220765756,17480318001653365239,131072 --disable-f
 2283 pts/0    R+     0:00 ps a

So AFAIK the start of electron is tested with this. And if you look into the remaining electron tests there is not really tested more.

[khassel]

Spectron is dead, here is the deprecation announcement.

Did a first test with playwright which was mentioned in the spectron issue, this works already:

const { _electron: electron } = require('playwright');

let electronApp;

describe("Electron app environment", function () {


        beforeAll(function () {
                // Set config sample for use in test
                process.env.MM_CONFIG_FILE = "tests/configs/env.js";
        });

        beforeEach(async function () {
                electronApp = await electron.launch({ args: ['js/electron.js'] });
        });

        afterEach(async function () {
                await electronApp.close();
        });

        it("should open a browserwindow", async function () {
                // Get the first window that the app opens, wait if necessary.
                const window = await electronApp.firstWindow();
                // Test the title.
                expect(await window.title()).toBe("MagicMirror²");
        });
});

So i will work on this to replace the remaining spectron tests with playwright.

[MichMich]

Champ!

[khassel]

From my side this can be closed.

[rejas]

Should this be closed now that #2784 is merged?

[rejas]

Oh, I opened it, so I guess I can ask myself :-) My verdict is yes :-)

[DanielHabenicht]

Hi @fewieden,

I created simple mocks for NodeHelper and Module, to avoid cloning magic mirror in the CI like in this example.

Did you publish the mocks anywhere? (except inside you repo)
If not, I am happy to publish a NPM package for it (or you can do it yourself), so it can be readily used in other packages. Or I provide a PR so it can be done from inside this repo?
Because I run into the same problem with my own package testing.

[fewieden]

@DanielHabenicht

Did you publish the mocks anywhere? (except inside you repo)

They are only in my repo https://github.com/fewieden/MMM-soccer/tree/master/__mocks__

If not, I am happy to publish a NPM package for it (or you can do it yourself), so it can be readily used in other packages. Or I provide a PR so it can be done from inside this repo?

I wouldn't add those mocks to the core project. Then you would end up with the same problem I tried to avoid: Cloning the project in the CI.

Those mocks currently only contain what I needed to test my module and only work if you use the jest test framework.

Feel free to publish them as a package.

Because I run into the same problem with my own package testing.

Do you have a link to your work in progress? You can also connect to me via the discord channel.