Convert load callbacks to async/await

[rejas]

Another one of these PRs....

[codecov-commenter]

Codecov Report

Merging #3050 (2d35d9e) into develop (498b440) will increase coverage by 0.06%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #3050      +/-   ##
===========================================
+ Coverage    23.00%   23.07%   +0.06%     
===========================================
  Files           52       52              
  Lines        11604    11593      -11     
===========================================
+ Hits          2670     2675       +5     
+ Misses        8934     8918      -16     

Impacted Files	Coverage Δ
js/loader.js	0.00% <0.00%> (ø)
js/module.js	0.00% <0.00%> (ø)
modules/default/calendar/node_helper.js	83.33% <0.00%> (-4.17%)	⬇️
modules/default/updatenotification/node_helper.js	90.27% <0.00%> (+12.50%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rejas]

Merge conflict fixed

[khassel]

I think the tests are not stable anymore. I know that the tests on github are very sensitive and that fails there are normal but I'm running own tests on gitlab after building new develop images and after this PR its the first time since months that this test fails.

I repeated the test run 3 times (all failed) and you can find the results here:

• https://gitlab.com/khassel/magicmirror/-/jobs/3818204533
• https://gitlab.com/khassel/magicmirror/-/jobs/3818339454
• https://gitlab.com/khassel/magicmirror/-/jobs/3818480669

Running locally I had similar errors but also successful runs, so it seems a transient problem.

[rejas]

Not good :-( And this happened after this PR? If so, we should undo that for now...

[khassel]

not sure but I ran also a pipeline before this PR which was successful

[rejas]

Can you check if my cleanup branch / PR improves the situation?

[khassel]

I tested now locally against your branch misc/cleanup and got 5 successful runs, no error.

Running this in ci is to complicated, we will see after merge ...

[rejas]

The #3051 looks a little more stable indeed on my pc. Shall we give it a try?

[khassel]

merged but not better, see https://gitlab.com/khassel/magicmirror/-/jobs/3836164426

We should be able to revert this before next release if there is no solution ...

[khassel]

there are simliar error in the node18 tests: https://github.com/MichMich/MagicMirror/actions/runs/4276168598/jobs/7444059604#step:4:1625

Another word to my setup, on my develop images (where above tests are running) I'm on node v19.

[khassel]

this becomes strange ...

I changed my pipeline to do the tests with node v16/18/19, you can see th results here.

The v16 pipeline succeeded, but the newer node versions fails.

[rejas]

The first error I see is something new for me:
"
ReferenceError: You are trying to import a file after the Jest environment has been torn down. From tests/e2e/modules/calendar_spec.js.
"

Any idea on this? I am on vacation this week so I am a little slow...

[khassel]

no idea at the moment, I cannot reporduce the reference error locally so may a timing problem.

[rejas]

So, what shall we do now with this?
I am probably a little busy this month and the next few too due to family expansion (delivery is expected around easter).
Shall we:

• keep it as is
• revert async commits

Opinions @sdetweil @khassel @MichMich ?

[MichMich]

As long as the tests don't break I'm all for it.

[MichMich]

More importantly: congrats with the planned family expansion! 😁

[rejas]

As long as the tests don't break I'm all for it.

well, they dont break a 100% of the time, the tests are just sometimes breaking. so we have to restart the checks until they run through without errors. Not very optimal...

[khassel]

and the next few too due to family expansion (delivery is expected around easter).

cool 👍

I have very limited time the whole week before next release ...

I'm unhappy with the current tests. I think we have 2 problems:

1. after the async changes the tests are failing for node versions >= 18 locally or in my gitlab tests (not on github because the jobs there are very slow)
2. some electron tests are failing only on github (restart mostly works)

AFAIS are both timing problems.

I try to find some time to test Nr. 1 by reverting the async commits, for Nr. 2 we can try to increase jest timeout from 20 to 30 sec.

The current situation must be solved, no one is looking deeper into tests which are failing with every run.

[khassel]

after the async changes the tests are failing for node versions >= 18 locally or in my gitlab tests (not on github because the jobs there are very slow)

this happens also on github, see https://github.com/MichMich/MagicMirror/actions/runs/4276168598/jobs/7444059604

Looking at all the fails this happens only in the 2 weather tests weather_forecast_spec.js and weather_hourly_spec.js in the for (const [index, xxx] construction.

The weather tests have own helper functions and a special construction to use mock data, may some of these things are causing these errors.