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

feat: support asynchronous config.set() call in karma.conf.js #3660

Merged

Conversation

npetruzzelli
Copy link
Contributor

@npetruzzelli npetruzzelli commented Feb 28, 2021

With this change, Karma allows to return a Promise from a configuration function directly or make such function async and thus allows to perform some asynchronous initialization/fetch configuration from somewhere. When the configuration function returns a Promise, Karma will wait until the promise is resolved before it proceeds with the next steps.

Example usage:

module.exports = async (config) => {
  const karmaConfig = await getKarmaConfig("dev");
  
  config.set({
    ...karmaConfig
  });
};

For Karma's end-users the change is fully backward compatible. For Karma's programmatic users the change is backward compatible, but the current behavior of parseConfig(), Server, runner, and stopper is deprecated and will change in the next major release. New behavior (which supports asynchronous configuration function) can be enabled with a flag. Please refer to the public API documentation for more details.

Fixes #1259

@karmarunnerbot
Copy link
Member

Build karma 540 completed (commit eeaff6b835 by @npetruzzelli)

@AppVeyorBot
Copy link

Build karma 2937 completed (commit eeaff6b835 by @npetruzzelli)

@npetruzzelli
Copy link
Contributor Author

@devoto13 - I'm not sure if I will be able to see this PR through to completion (quickly), but I was able to at least start the work so that the conversation/review can start.

I can't help but to "throw my hat into the ring" as it were. I initially opened #3631 / #3635 because I was working on a personal module that would support ES Modules and Promises. It still needs a lot of work, including the addition of tests (which is why it is only v 0.1.0). It is significantly overbuilt for what Karma would want or need, but I'll link it here in case you want to have a look:
https://github.com/promise-file/promise-file/tree/v0.1.0/packages/promise-file-karma-config

@karmarunnerbot
Copy link
Member

Build karma 539 completed (commit eeaff6b835 by @npetruzzelli)

@npetruzzelli
Copy link
Contributor Author

Would this need the ability for users to pass their own promise implementation, or will the default Promise meet the needs of all environments we expect this to serve in?

@karmarunnerbot
Copy link
Member

Build karma 541 completed (commit 527337b0be by @npetruzzelli)

@AppVeyorBot
Copy link

Build karma 2938 completed (commit 527337b0be by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 540 completed (commit 527337b0be by @npetruzzelli)

@AppVeyorBot
Copy link

Build karma 2939 completed (commit ff32cf5f74 by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 541 completed (commit ff32cf5f74 by @npetruzzelli)

@npetruzzelli
Copy link
Contributor Author

ASIDE: Should the process and parseClientArgs functions from cli.js be made a part of the public API for anyone using parseConfig and the public API methods directly? The intent being that they may want to parse command line arguments in a way that is consistent with Karma (this deserves its own issue for discussion prior to a PR and is not in the scope of this PR.)

@npetruzzelli
Copy link
Contributor Author

It still needs new tests to be written, but I am marking this as ready for review to address feedback prior to putting time into tests so that I can avoid writing tests that may be made obsolete by review feedback.

@npetruzzelli npetruzzelli marked this pull request as ready for review March 2, 2021 16:24
@npetruzzelli npetruzzelli changed the title [WIP] Add Promise support to parseConfig Add Promise support to parseConfig Mar 2, 2021
@npetruzzelli npetruzzelli changed the title Add Promise support to parseConfig Add Promise Support To parseConfig Mar 2, 2021
@karmarunnerbot
Copy link
Member

Build karma 542 completed (commit ff32cf5f74 by @npetruzzelli)

@devoto13
Copy link
Collaborator

devoto13 commented Mar 4, 2021

I'm not sure if I will be able to see this PR through to completion (quickly), but I was able to at least start the work so that the conversation/review can start.

No worries, take your time. And thanks for the work you're putting into implementing this!

Would this need the ability for users to pass their own promise implementation, or will the default Promise meet the needs of all environments we expect this to serve in?

Default implementation is enough.

Should the process and parseClientArgs functions from cli.js be made a part of the public API for anyone using parseConfig and the public API methods directly? The intent being that they may want to parse command line arguments in a way that is consistent with Karma (this deserves its own issue for discussion prior to a PR and is not in the scope of this PR.)

I'm not sure how useful is this. Better have strong real use case for this and discuss it before doing anything.

It still needs new tests to be written, but I am marking this as ready for review to address feedback prior to putting time into tests so that I can avoid writing tests that may be made obsolete by review feedback.

I'll take a look right away.

Copy link
Collaborator

@devoto13 devoto13 left a comment

Choose a reason for hiding this comment

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

It looks great @npetruzzelli! Thanks a lot for doing this.

I left a couple of tiny comments. Let me know if they make sense to you.

I've also checked point (4). Looking at the PR code, I don't think we need to do anything about it.

lib/config.js Outdated Show resolved Hide resolved
lib/cli.js Outdated Show resolved Hide resolved
lib/cli.js Outdated Show resolved Hide resolved
lib/server.js Show resolved Hide resolved
@karmarunnerbot
Copy link
Member

Build karma 546 completed (commit b08ab63426 by @npetruzzelli)

@AppVeyorBot
Copy link

Build karma 2943 completed (commit b08ab63426 by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 545 completed (commit b08ab63426 by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 561 completed (commit cbdb405e59 by @npetruzzelli)

Copy link
Collaborator

@devoto13 devoto13 left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left couple of comments to cleanup.

test/unit/config.spec.js Outdated Show resolved Hide resolved
test/unit/config.spec.js Outdated Show resolved Hide resolved
lib/cli.js Outdated Show resolved Hide resolved
@karmarunnerbot
Copy link
Member

Build karma 565 completed (commit 936aa92bba by @npetruzzelli)

@AppVeyorBot
Copy link

Build karma 2962 completed (commit 936aa92bba by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 564 completed (commit 936aa92bba by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 565 completed (commit 65f3b8fbbd by @npetruzzelli)

@karmarunnerbot
Copy link
Member

Build karma 566 completed (commit 65f3b8fbbd by @npetruzzelli)

@npetruzzelli
Copy link
Contributor Author

npetruzzelli commented Mar 22, 2021

@devoto13 - While working on updates for this round of review, I was running into an issue where random tests in config.spec.js would log WARN [config]: urlRoot normalized to "/".

  • Sometimes it happens, sometimes it doesn't (making it difficult to replicate)
  • Sometimes it was the same test, sometimes it wasn't
  • It always only happened once per run
  • It was never any of the parseConfig tests
  • Using .only to reduce the number of tests made it much less likely to occur, though using .only on the top level describe didn't seem to affect it
  • The only console message that (currently) interrupts mocha's status reporter is: (node:21192) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification. which has been the norm for me since I started contributing.

I haven't been able to replicate the problem as of the latest commit. It may not be an issue anymore, but I wanted to make you aware of it.

@devoto13 devoto13 changed the title Add Promise Support To parseConfig feat: support asynchronous config.set() call in karma.conf.js Mar 23, 2021
@devoto13 devoto13 mentioned this pull request Mar 23, 2021
17 tasks
Copy link
Collaborator

@devoto13 devoto13 left a comment

Choose a reason for hiding this comment

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

I've updated the title and description to provide some useful information to future readers. It looks good to go now.

@npetruzzelli Thanks for doing all the hard work! 💪

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

awesome, thanks

@johnjbarton johnjbarton merged commit 4c9097a into karma-runner:master Mar 23, 2021
karmarunnerbot pushed a commit that referenced this pull request Mar 23, 2021
# [6.3.0](v6.2.0...v6.3.0) (2021-03-23)

### Features

* support asynchronous `config.set()` call in karma.conf.js ([#3660](#3660)) ([4c9097a](4c9097a))
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@npetruzzelli
Copy link
Contributor Author

You're most welcome! Glad to help!

nicojs added a commit to nicojs/karma that referenced this pull request Feb 11, 2022
jginsburgn pushed a commit to nicojs/karma that referenced this pull request Apr 8, 2022
jginsburgn pushed a commit that referenced this pull request Apr 8, 2022
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
…a-runner#3660)


The existing sync behavior co-exists with the new async behavior.

* add promise support to `parseConfig`
* add async config support to `cli`, `runner`, and `stopper`
* Additional API for `Server()` accepting parsed config.
  Older API is deprecated.
* update documentation for parseConfig
* add warning for deprecated use of CLI options
*  update Server constructor, runner, and stopper docs
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
# [6.3.0](karma-runner/karma@v6.2.0...v6.3.0) (2021-03-23)

### Features

* support asynchronous `config.set()` call in karma.conf.js ([karma-runner#3660](karma-runner#3660)) ([4c9097a](karma-runner@4c9097a))
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling config.set asynchronously not working
5 participants