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

[Proposal] rootHooks accept import(name) as input #4780

Closed
loynoir opened this issue Oct 28, 2021 · 4 comments
Closed

[Proposal] rootHooks accept import(name) as input #4780

loynoir opened this issue Oct 28, 2021 · 4 comments
Labels
status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior type: feature enhancement proposal

Comments

@loynoir
Copy link

loynoir commented Oct 28, 2021

Is your feature request related to a problem or a nice-to-have?? Please describe.

Background: karma & mocha

karma-mocha is not await for <script type="module">. When XXX.spec.js have top level await, or slow import, all unit tests within it are silently missing. issue

rootHooks works in mocha

I fixed it in pull. Try to add unit test. And mocha can identify if some unit tests are missing.

mocha.setup({
    ui: "bdd",
    rootHooks: {afterAll},
})

rootHooks not works in karma

Got serialize problem. rootHooks in mocha.setup(config) is not serializable.

// karma.config.js
client: {
    mocha: {
        timeout: 600000,
        rootHooks: {afterAll}
    },
}
// karma runtime context
> window.__karma__.config.mocha
{ timeout: 600000, rootHooks: {} }

Describe the solution you'd like

Related defination, add here for better reading

interface RootHookObject {
  afterAll?: Func | AsyncFunc | Func[] | AsyncFunc[] | undefined;
  beforeAll?: Func | AsyncFunc | Func[] | AsyncFunc[] | undefined;
  afterEach?: Func | AsyncFunc | Func[] | AsyncFunc[] | undefined;
  beforeEach?: Func | AsyncFunc | Func[] | AsyncFunc[] | undefined;
}

Current defination

interface MochaOptions {
  timeout?: number | string | undefined;
  ui?: Interface | undefined;
  // ...

  rootHooks?: Mocha.RootHookObject | undefined;
}

Proposal

interface MochaOptions {
  timeout?: number | string | undefined;
  ui?: Interface | undefined;
  // ...

  rootHooks?:
    Mocha.RootHookObject
    | undefined
    | Promise<{ mochaHooks: Mocha.MochaOptions }>
    | { type: "import", value: string }
}

Usage

Serializable configuration

// karma.config.js
client: {
    mocha: {
        timeout: 600000,
        rootHooks: { type: "import", value: "/path/to/my/mochaHooks.mjs" }
    },
}

Equals to

mocha.setup({
  rootHooks: { type: "import", value: "/path/to/my/mochaHooks.mjs" }
})

Equals to

mocha.setup({
  rootHooks: import("/path/to/my/mochaHooks.mjs")
})
  1. import() supported by node and browsers, and can use modules.
  2. As unit test describe() in sync context, results in mocha.setup can't await here.

Describe alternatives you've considered
On karma-mocha side, using AST tools convert rootHooks to sync version browser support code, and somehow load all dynamic files into karma, before mocha.setup and all karma files.

Additional context

@loynoir loynoir added the type: feature enhancement proposal label Oct 28, 2021
@juergba
Copy link
Member

juergba commented Oct 30, 2021

@loynoir we are using karma-mocha in our repo as well and I'm not happy with it, since it hasn't been maintained for one year and a half. Its status is completely outdated. I always feel releived when our CI karma tests have passed successfully. So my enthusiasm to touch Mocha for karma-mocha's sake is limited.

karma-mocha is not await for <script type="module">: to load ESM test files, I guess. I don't understand why you want to abuse hooks/rootHooks to load ESM test files. Why this complicated detour? Why not make karma-mocha inject ESM modules the correct way?

Got serialize problem: ??

In Mocha the mochaHooks are loaded via --require plugin which then returns the rootHooks object. In the browser you directly work with the rootHooks object. To extend the rootHooks with an import feature seems weird to me.
Wouldn't this way be easier and more obvious?

// karma.config.js
 client: {
      mocha: {
        require: './hooks.js'
      }
 }

Btw Mocha's require option accepts CJS and ESM modules.

Tbh I don't really understand this PR. I don't see the link between rootHooks and ESM test files.

@loynoir
Copy link
Author

loynoir commented Oct 30, 2021

we are using karma-mocha in our repo as well and I'm not happy with it, since it hasn't been maintained for one year and a half. Its status is completely outdated. I always feel releived when our CI karma tests have passed successfully. So my enthusiasm to touch Mocha for karma-mocha's sake is limited.

Sorry, I just notice mocha hasn't been maintained for one year and a half.

I'm very appreciate for your maintainance here. 👍

I'm ok to close this proposal.

Got serialize problem: ??

karma seems can not parse function from karma.config.js to runtime context.

// karma.config.js
client: {
    mocha: {
        timeout: 600000,
        // afterAll is a function
        rootHooks: {afterAll}
    },
}
// karma runtime context
// window.__karma__.config.mocha.rootHooks is empty object
> window.__karma__.config.mocha
{ timeout: 600000, rootHooks: {} }

Why not make karma-mocha inject ESM modules the correct way?

Yes, before this proposal, I already fix that in my karma-mocha fork.

Run mocha.run after all .mjs are awaited, so none describe() is silently left behind.

I don't see the link between rootHooks and ESM test files.

I use rootHooks.afterAll to test my fix work, karma-mocha not running mocha.run too early, describe() in .mjs in run at least one.

require: './hooks.js'

Thanks for your suggestion. Anyway, I have got test working, to ensure no describe() is left behind.


At last, appreciate again, for your mocha maintainance 🤗

@juergba
Copy link
Member

juergba commented Nov 1, 2021

@loynoir thank you 👍
I'm going to close this issue then.

@juergba juergba closed this as completed Nov 1, 2021
@juergba juergba added the status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior label Nov 1, 2021
@jginsburgn
Copy link

I just want to leave a note here that the Karma repos are still being maintained but with limited resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior type: feature enhancement proposal
Projects
None yet
Development

No branches or pull requests

3 participants