Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the e2e tests wait for the app to start and close before running next test #2952

Merged
merged 5 commits into from
Oct 29, 2022

Conversation

buxxi
Copy link
Contributor

@buxxi buxxi commented Oct 18, 2022

When trying to debug why the tests broke for #2946 I found that the tests does not wait for the app to start and close. So if the startup isn't blocking that would fail.

So I added a callback for close() too and converted them to promises for the startApplication() and stopApplication() and updated all the e2e tests to await both. Will try to refactor all these callbacks to promises in a later PR.

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

Merging #2952 (cf5dff5) into develop (dde8860) will decrease coverage by 42.07%.
The diff coverage is 70.43%.

@@             Coverage Diff              @@
##           develop    #2952       +/-   ##
============================================
- Coverage    65.21%   23.13%   -42.08%     
============================================
  Files           14       48       +34     
  Lines          733    10026     +9293     
============================================
+ Hits           478     2320     +1842     
- Misses         255     7706     +7451     
Impacted Files Coverage Δ
js/server.js 78.61% <68.80%> (+11.04%) ⬆️
js/app.js 84.03% <100.00%> (+14.50%) ⬆️
js/electron.js 0.00% <0.00%> (-70.97%) ⬇️
js/logger.js 46.83% <0.00%> (-5.80%) ⬇️
js/utils.js 100.00% <0.00%> (ø)
js/deprecated.js 100.00% <0.00%> (ø)
modules/default/defaultmodules.js 100.00% <0.00%> (ø)
...s/default/updatenotification/updatenotification.js 0.00% <0.00%> (ø)
modules/default/alert/alert.js 0.00% <0.00%> (ø)
modules/default/weather/providers/weathergov.js 0.00% <0.00%> (ø)
... and 40 more

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

@rejas rejas requested a review from khassel October 18, 2022 18:44
@buxxi
Copy link
Contributor Author

buxxi commented Oct 18, 2022

Hmm, it's so strange that it only fails on 18.x, seems to be when it uses the built in fetch instead of node-fetch. Changing that and it works...

@khassel
Copy link
Collaborator

khassel commented Oct 19, 2022

Hmm, it's so strange that it only fails on 18.x, seems to be when it uses the built in fetch instead of node-fetch. Changing that and it works...

running the tests locally with npm run test:e2e the one ipWhitelist test fails as in the automated tests here.

But running the single test with npx jest tests/e2e/ipWhitelist_spec.js works ...

@rejas
Copy link
Collaborator

rejas commented Oct 19, 2022

Maybe something isnt shutdown correctly before the one test?

@khassel
Copy link
Collaborator

khassel commented Oct 19, 2022

Maybe something isnt shutdown correctly before the one test?

yes, but it not depends on which test is running before, had different tests as predecessor ...

@khassel
Copy link
Collaborator

khassel commented Oct 19, 2022

if I put back the ugly "wait" in stopApplication it works ...

exports.stopApplication = async () => {
        if (global.app) {
                return new Promise((resolve, reject) => {
                        global.app.stop(resolve);
                });
        }
        await new Promise((resolve) => setTimeout(resolve, 100));
        return Promise.resolve();
};

@buxxi
Copy link
Contributor Author

buxxi commented Oct 19, 2022

The fetch fails and there's no catch() in the wrapper in global-setup.js and that causes the test to wait forever for the resolve to happen (which it doesn't).

Using the following it fails more in a proper way:

exports.fetch = (url) => {
	return corefetch(url);
};

But haven't yet found out why the fetch fails with native in node 18 and why it works with node-fetch...

I wont be able to look into this for a week or so.

@buxxi
Copy link
Contributor Author

buxxi commented Oct 28, 2022

Changed the tests to use node-fetch instead of the built-in fetch for node >= 18 because the built-in sets the header Connection: keep-alive between all requests which messes with the server starting up and shutting down between all the tests. Tried setting the header manually and setting a separate agent without any success. And it always logs that fetch is still an experimental feature, so I think we should treat it as that...

Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I would just like you to fill the missing jsdoc entries.

@rejas
Copy link
Collaborator

rejas commented Oct 29, 2022

Not sure if I requests @khassel to review, otherwise I would merge it

@khassel
Copy link
Collaborator

khassel commented Oct 29, 2022

with this we are using internal node fetch command (for node >=18) in the default modules but the tests are running with node-fetch instead. Means for me these are no tests for the modules (for node >=18).

@buxxi
Copy link
Contributor Author

buxxi commented Oct 29, 2022

  • If the tests call production code on node >= 18 the internal fetch will still be used, it's just the fetches that is used only in the test code that would use node-fetch
  • I don't believe the internal fetch is production ready and should probably not be used at all (yet) because:
    • Running the following:
     node -e "global.fetch('http://en.wikipedia.org').then((res) => console.log(res.status))"
    Produces:
     (node:71384) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
     (Use `node --trace-warnings ...` to show where the warning was created)
     200
    
  • Having tests depend on a randomly selected timeout and sometimes fail is really annoying, had that happen to me while debugging this with the old 200ms timeout.

@khassel
Copy link
Collaborator

khassel commented Oct 29, 2022

I don't believe the internal fetch is production ready and should probably not be used at all (yet)

that was my point, I don't like it when we migrate the tests back but the rest not. If the internal fetch is now seen as not production ready it should be removed completly (and may evaluated later again, it is still experimental in node v19)

@khassel
Copy link
Collaborator

khassel commented Oct 29, 2022

but from my side we can merge this and remove internal fetch in another PR

@rejas
Copy link
Collaborator

rejas commented Oct 29, 2022

I am not so deep into the whole node/fetch thing but maybe what is done in the other PR (1c8ea72)
would be a solution?

let nodefetch;
		if (nodeVersion < 18) {
			jest.mock("node-fetch");
			nodefetch = require("node-fetch");
		}
		let fetchMock;

But that is just my "gefährliches halbwissen" (dagnerously half-knowing :-)

@buxxi
Copy link
Contributor Author

buxxi commented Oct 29, 2022

Mocking the requests would probably make most of the tests useless as they verify the response from the server. Don't think that solution can be applied here.

I agree that it's better to do it in another PR.

Guess we have to wait a bit longer to get rid of that extra dependency :)

@rejas
Copy link
Collaborator

rejas commented Oct 29, 2022

I agree. It would be nice to get rid of a dependency but we should play it safe.
So, whats the plan now? First a PR for getting rid of internal fetch, or first merge your PR?

@khassel
Copy link
Collaborator

khassel commented Oct 29, 2022

you can merge.

The other change would be here. I would suggest to comment out the if/else (but leave it in) and return always the node-fetch part. So we have to change nothing else and can insert the internal or other fetch functions there when needed.

can do this PR the next days

@rejas rejas merged commit f25abfd into MagicMirrorOrg:develop Oct 29, 2022
@rejas
Copy link
Collaborator

rejas commented Oct 29, 2022

can do this PR the next days

Or I do it on a saturday evening? #2961 ;-)

khassel pushed a commit that referenced this pull request Oct 29, 2022
As discussed in #2952

Co-authored-by: veeck <michael@veeck.de>
@buxxi buxxi deleted the e2e-tests-async branch October 30, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants