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

🐛 BUG: mockOAuthFlow breaks Miniflare in tests #6132

Closed
geelen opened this issue Jun 24, 2024 · 2 comments · Fixed by #6174
Closed

🐛 BUG: mockOAuthFlow breaks Miniflare in tests #6132

geelen opened this issue Jun 24, 2024 · 2 comments · Fixed by #6174
Assignees
Labels
bug Something that isn't working

Comments

@geelen
Copy link
Contributor

geelen commented Jun 24, 2024

Which Cloudflare product(s) does this pertain to?

Wrangler core, Miniflare

What version(s) of the tool(s) are you using?

3.60.3

What version of Node are you using?

20.10.0

What operating system and version are you using?

Mac 14.5

Describe the Bug

In testing #6073, I had test failures for this production code:

const mf = new Miniflare({
    modules: true,
    script: "export default {}",
    d1Persist,
    d1Databases: { DATABASE: id },
});

The error was:

 FAIL  src/__tests__/d1/export.test.ts > execute > should handle local
TypeError: server.on is not a function
 ❯ module.exports ../../node_modules/.pnpm/stoppable@1.1.0/node_modules/stoppable/lib/stoppable.js:14:12
 ❯ ../miniflare/src/runtime/config/workerd.capnp.js:8716:51
 ❯ Miniflare2.#startLoopbackServer ../miniflare/src/runtime/config/workerd.capnp.js:8715:12
 ❯ Miniflare2.#getLoopbackPort ../miniflare/src/runtime/config/workerd.capnp.js:8708:59
 ❯ Miniflare2.#assembleAndUpdateConfig ../miniflare/src/runtime/config/workerd.capnp.js:8934:53
 ❯ ../miniflare/src/runtime/config/workerd.capnp.js:8555:87
 ❯ Mutex.runWith ../miniflare/src/runtime/config/workerd.capnp.js:3498:25
 ❯ new Miniflare2 ../miniflare/src/runtime/config/workerd.capnp.js:8555:44
 ❯ exportLocal src/d1/export.ts:131:13
    129|  );
    130|
    131|  const mf = new Miniflare({

I jumped into Miniflare to figure out which line was breaking, it was this invocation:

#startLoopbackServer(hostname: string): Promise<StoppableServer> {
    if (hostname === "*") hostname = "::";
    
    return new Promise((resolve) => {
	    const server = stoppable(
		    http.createServer(this.#handleLoopback),
		    /* grace */ 0
	    );
	    server.on("upgrade", this.#handleLoopbackUpgrade);
	    server.listen(0, hostname, () => resolve(server));
    });
}

Turns out, http.createServer was returning a mock object which had no method .on (which was required by stoppable, otherwise it would have failed a few lines later). That was a side effect of my test calling:

import { mockOAuthFlow } from "../helpers/mock-oauth-flow";

// ...

const { mockOAuthServerCallback } = mockOAuthFlow();

Which, it turns out, I didn't need. So I deleted that and my tests were fine. But adding a mock for oAuth shouldn't break Miniflare, should it? So maybe there's a better way to do that?

Please provide a link to a minimal reproduction

No response

Please provide any relevant error logs

No response

@geelen geelen added the bug Something that isn't working label Jun 24, 2024
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jun 24, 2024
@petebacondarwin
Copy link
Contributor

I think in general we should not need to use this mock handler. It should only be needed for specific login flow tests.
I think we can probably go and remove its use from all the D1 tests.

@petebacondarwin
Copy link
Contributor

Indeed. I have removed the usage in #6174 - and added a comment to discourage you from trying to use it in the future 👯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants