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

Change deno tests to also use MMS #12918

Merged
merged 5 commits into from
Feb 13, 2023
Merged

Conversation

hasezoey
Copy link
Collaborator

@hasezoey hasezoey commented Jan 17, 2023

Summary

Follow-up to #12890

This PR changes the deno tests and CI to also use MMS for easier testing

WIP because:

Deno permissions changes:

  • added --allow-write because MMS needs to save the downloaded binary and create the folders
  • added --unstable because TCP.unref is currently still a unstable function (and maybe some other unstable function, but i have not passed further than a TCP.unref error)

@hasezoey
Copy link
Collaborator Author

hasezoey commented Jan 17, 2023

note that the deno test may suddenly fail randomly, see https://github.com/hasezoey/mongoose/actions/runs/3940878098/jobs/6742544402 (updated, before i mentioned the wrong run)

  3314 passing (1m)
  54 pending

error: Uncaught Error: connect ECONNREFUSED 127.0.0.1:37227 - Local (undefined:undefined)
    Error.captureStackTrace(err);
          ^
    at __node_internal_captureLargerStackTrace (https://deno.land/std@0.173.0/node/internal/errors.ts:113:11)
    at __node_internal_exceptionWithHostPort (https://deno.land/std@0.173.0/node/internal/errors.ts:295:12)
    at TCPConnectWrap._afterConnect [as oncomplete] (https://deno.land/std@0.173.0/node/net.ts:369:16)
    at TCP.afterConnect (https://deno.land/std@0.173.0/node/internal_binding/connection_wrap.ts:73:11)
    at https://deno.land/std@0.173.0/node/internal_binding/tcp_wrap.ts:383:16
Error: Process completed with exit code 1.

i cannot locally reproduce this (particular) connection refused error and so makes it hard to locate where the connection does not get properly closed

also this error will always happen in deno, because in there the tests and the mocha instance running share the same context from what i can tell, so the connections that are not closed will still be existing before the instance is shut-down and so throw errors (and are hard to locate where the connection was created). in normal mocha (non-deno tests) it works because the tests themself and the fixtures run in different context (from what i can tell) so the tests are closed (with connections) before the instance stop is run and so do not cause errors

possible solutions to at least make debugging easier:

  • name connections (maybe based on the test name) and include that in all connection related errors *1
  • refactor (and look through) all tests and ensure everything gets closed correctly (and maybe not too many extra connections)

*1 example how naming may work:

// PSEUDO CODE
before(() => {
  connection = new connection().withName(this.title + " before");
}
beforeEach(() => {
  connection = new connection().withName(this.currentTest.fullTitle());
}

@hasezoey
Copy link
Collaborator Author

Update: TCP.unref had the stabilization PR merged and will (probably) be part of deno 1.30.0
denoland/deno#17477

@hasezoey
Copy link
Collaborator Author

hasezoey commented Feb 7, 2023

deno 1.30.0 is released, which stabilizes TCP.unref(), and mentioned updates have been done in d9a6de7

@hasezoey hasezoey marked this pull request as ready for review February 7, 2023 07:35
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@vkarpov15 vkarpov15 merged commit b3e461a into Automattic:master Feb 13, 2023
@vkarpov15 vkarpov15 added this to the 6.9.2 milestone Feb 13, 2023
@hasezoey hasezoey deleted the denoToMMS branch February 13, 2023 18:45
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.

None yet

2 participants