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

Passing serializable data from globalSetup to tests #4025

Closed
4 tasks done
luixo opened this issue Aug 26, 2023 · 12 comments · Fixed by #4422
Closed
4 tasks done

Passing serializable data from globalSetup to tests #4025

luixo opened this issue Aug 26, 2023 · 12 comments · Fixed by #4422

Comments

@luixo
Copy link

luixo commented Aug 26, 2023

Clear and concise description of the problem

The problem

I would like to use a single resource (say, a server with a DB) for all the tests, but I need to pass that server (in a form of hostname / port) to the tests.
I'm aware that passing actual instances to a worker is somewhere between smelly and impossible, so I want to pass a small bit of serializeable data, hostname and port in my case.

globalSetup and the data

Unfortunately, globalSetup files don't get any data from the outside world (Jest counterparty does actually get globalConfig and projectConfig as arguments), so I made a tiny patch to fix that:

# packages/vitest/src/node/plugins/globalSetup.ts#L59-65

      try {
        for (const globalSetupFile of globalSetupFiles) {
-         const teardown = await globalSetupFile.setup?.()
+         const teardown = await globalSetupFile.setup?.(project)
          if (teardown == null || !!globalSetupFile.teardown)
            continue
          if (typeof teardown !== 'function')
# packages/vitest/src/node/plugins/globalSetup.ts#L78-84
      if (globalSetupFiles?.length) {
        for (const globalSetupFile of globalSetupFiles.reverse()) {
          try {
-           await globalSetupFile.teardown?.()
+           await globalSetupFile.teardown?.(project)
          }
          catch (error) {
            logger.error(`error during global teardown of ${globalSetupFile.file}`, error)

Passing data to the test

I found that I can pass environmentOptions to the environment which later can inject whatever needs to be injected directly in the global scope of the test (as well as setup file, I believe it is the same global scope).
I didn't find a way to modify those environmentOptions other way than adding those to configOverrides prop of the project config, so I ended up doing that.

My setup to achieve data passing around looked like this:
vitest.config.ts

import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    globalSetup: "./tests/global.setup.ts",
    environment: "./tests/environment.ts",
    setupFiles: "./tests/tests.setup.ts",
    experimentalVmThreads: true,
  },
});

global.setup.ts

import type { Vitest } from "vitest";

declare module "vitest" {
  interface EnvironmentOptions {
    serverConfig: {
      port: number;
      hostname: string;
    };
  }
}

export default async (vitest: Vitest) => {
  const serverConfig = {
    // implementation is not important
    port: await getFreePort(),
    hostname: "localhost",
  };
  // Passing the data to the environment, it would be nice to have a less hacky way to do that
  vitest.configOverride.environmentOptions = { serverConfig };
  // implementation is not important
  await startServer(serverConfig);
};

environment.ts

import vm from "node:vm";
import type { Environment, EnvironmentOptions } from "vitest";
import { builtinEnvironments } from "vitest/environments";

declare global {
  // That smells as it will be visible in every global context being used in only one
  var serverConfig: EnvironmentOptions["serverConfig"];
}

export default <Environment>{
  name: "custom",
  transformMode: "ssr",
  setupVM: async (options: EnvironmentOptions) => {
    const { getVmContext, teardown } = await builtinEnvironments.node.setupVM!(
      options,
    );
    const context = getVmContext();
    const globalInContext: typeof global = vm.runInContext("this", context);
    globalInContext.serverConfig = options.serverConfig;
    return {
      getVmContext: () => context,
      teardown,
    };
  },
  // not used with experimentalVmModules
  setup: () => ({ teardown: () => {} }),
};

tests.setup.ts

import { beforeAll, beforeEach } from "vitest";

const { port, hostname } = globalThis.serverConfig;
// createClient implementation is not important
const client = createClient(`http://${hostname}:${port}`);

// .. or whichever way you need to use that passed data
beforeAll(async () => {
  const ref = await client.setupSuite();
  return async () => {
    await client.destroySuite(ref);
  }
});

beforeEach(async () => {
  const ref = await client.setupTest();
  return async () => {
    await client.destroyTest(ref);
  }
});

Cutting the (environment) middleman

I didn't like that I used environment as a middleman to just pass the data to the test, so I started digging.
I found that ResolvedConfig could actually be found in globalThis.__vitest_worker__.config in the global scope of a worker, so I removed the environment and a final version of tests.setup.ts looks like this:

import { beforeAll, beforeEach, ResolvedConfig } from "vitest";

const config = globalThis.__vitest_worker__.config as ResolvedConfig;
const { port, hostname } = config.environmentOptions.routerConfig;
// createClient implementation is not important
const client = createClient(`http://${hostname}:${port}`);

// The same as before
...

Suggested solution

I believe the main idea behind not letting data pass between a Vitest global scope and test worker global scope was to prevent leaking.
The data flow between those two scopes is not hard to do (considering Vitest not passing any data to globalSetup is a quirk, not a reasonable decision), so I suggest we can implement a more user-friendly way to pass it around (maybe a serializeability check can be added to add more precautionary measures).

Alternative

Keep my hacky solution as it is.

Additional context

There was an issue similar to this one and the solution provided was using filesystem to pass serializable data around (jest-puppeteer made this suggestion popular).
I believe that's not the way (it adds one more point of potential errors), managing resources should be easier.

I don't have much experience using workers and VM contexts, but it even could be a way to pass handles around so we can have a strictly typed messaging system between Vitest global scope and test worker global scope or pass actual object handles around.

Validations

@sheremet-va
Copy link
Member

I think it might be nice to have some kind of inject/provide API for this:

// globalSetup.js
export function setup({ provide }) {
  provide('router', { port: 3000, hostname: 'localhost' })
}
// inject.test.js
import { vi, expect } from 'vitest'

expect(vi.inject('router')).toEqual({ port: 3000, hostname: 'localhost' })

It can be also used in reporters or during other lifecycle hooks (like in reporters) for usage by libraries (@nicojs do you think this might also be useful for you?):

export default {
  test: {
    reporters: [
      {
        onInit(ctx) { ctx.provide('some-data', { data: true }) }
      }
    ]
  }
}

@luixo
Copy link
Author

luixo commented Aug 27, 2023

vi.inject('router')

Do you want the injection to be explicit? Is there a downside of injecting it by default on worker init (like config is currently being injected)?

I would greatly appreciate an ability to extend the types (via declare module "vitest" as I described in the first post) to keep ts slapping my wrist in case I change the value signature.
If it would not break the current API, I would even suggest to make the injected values mandatory on setup return (but this may be available only on next major).

@sheremet-va
Copy link
Member

Do you want the injection to be explicit? Is there a downside of injecting it by default on worker init (like config is currently being injected)?

The user explicitly defines what should be injected, and then it will initiated in the worker init phase.

I would greatly appreciate an ability to extend the types (via declare module "vitest" as I described in the first post) to keep ts slapping my wrist in case I change the value signature.

Sure, it can be typed like:

interface VitestProvide {
  router: { port: number }
}

vi.inject('router') // { port: number }

If it would not break the current API, I would even suggest to make the injected values mandatory on setup return (but this may be available only on next major).

I don't understand what you mean. The return value is a teardown function.

@luixo
Copy link
Author

luixo commented Aug 29, 2023

The user explicitly defines what should be injected, and then it will initiated in the worker init phase.

I mean the vi.inject("router") in the test itself. Is there a reason why not just use testContext.provided.router for example?

Sure, it can be typed like

👍

I don't understand what you mean. The return value is a teardown function.

Yep, that's why I mention breaking change.
It could be something like that:

// global.setup.ts
export default async () => {
  // setup
  return {
    teardown: async () => {...},
    data: { ... } // this is provided data
  }
}

This will break having multiple global setups though, so I don't think it will work.

@sheremet-va
Copy link
Member

Is there a reason why not just use testContext.provided.router for example?

But why limit the usage only to a test method? What if you need to access it outside of the test? In beforeAll, for example, which doesn't have access to text context.

It could be something like that

I don't like this because it's a very limited solution and it doesn't really explain what data is and why do you use it while inject/provide can be used universally to pass data from the main thread to workers

@luixo
Copy link
Author

luixo commented Aug 29, 2023

But why limit the usage only to a test method? What if you need to access it outside of the test?

Absolutely agree.

In beforeAll, for example, which doesn't have access to text context.

Is there a reason to not have test context there (maybe a suite context)?
I will have another issue open on the hack I had to do to pass data from beforeAll to the tests :)

@sheremet-va
Copy link
Member

Is there a reason to not have test context there (maybe a suite context)?

Because context is created for each test individually, but beforeAll runs only once before any context is created.

@luixo
Copy link
Author

luixo commented Aug 29, 2023

Because context is created for each test individually, but beforeAll runs only once before any context is created.

It does make sense.
Do you think there's a way a chain of hooks might pass the data to each other?
Something like this:

// first beforeAll
beforeAll(async (context) => {
  // sets context.beforeAllFirstData
})

// last beforeAll
beforeAll(async (context) => {
  // uses context.beforeAllFirstData
  // sets context.beforeAllLastData
})

// first beforeEach
beforeEach(async (context) => {
  // uses context.beforeAllFirstData
  // uses context.beforeAllLastData
  // sets context.beforeEachFirst
})

// last beforeEach
beforeEach(async (context) => {
  // uses context.beforeAllFirstData
  // uses context.beforeAllLastData
  // uses context.beforeEachFirst
  // sets context.beforeEachLast
})

test(async () => {
  // uses context.beforeAllFirstData
  // uses context.beforeAllLastData
  // uses context.beforeEachFirst
  // uses context.beforeEachLast
});
// .. and the same with afterEach / afterAll

This is kinda out of the scope for this issue though, do you want me to open a separate discussion on that topic?

@sheremet-va
Copy link
Member

This is kinda out of the scope for this issue though, do you want me to open a separate discussion on that topic?

As I said, this is not possible since context is unique to a test (and it creates a new one for before each test) and doesn't exist outside of it, so you don't have access to it in these hooks.

@luixo
Copy link
Author

luixo commented Oct 31, 2023

@sheremet-va
Should we consider the concept approved?
I can do my best to open a PR for that (for now I live with a patched package in my project).

@sheremet-va
Copy link
Member

Should we consider the concept approved?

Do you mean inject/provide? Yes, should be straightforward to implement.

@luixo
Copy link
Author

luixo commented Nov 1, 2023

Do you mean inject/provide? Yes, should be straightforward to implement.

Great, do you (or someone else among maintainers) have capacity to implement it or plan to leave for the community?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants