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

Support extension settings ('pretest', 'packageManager', 'testCommand', 'extraArguments', and 'executableOptions') #11

Merged
merged 5 commits into from
Mar 24, 2024

Conversation

cafesanu
Copy link
Contributor

@cafesanu cafesanu commented Mar 11, 2024

Hey @rluvaton. First, thanks for this extension! Truly the only one that worked for us.

Though I would help by adding extension settings.

  • The PR adds the possiblity to add settings to change certain behaviours while being backwards compatiple. If user does not add any settings, extention will keep working as before.
  • Since the run and watch functions were the same except for the "run" or "watch" string, I merged them into one and now the string "run" or "watch" can be passed to the function.

Extension Settings

Command Description Examples Default
vscode-vitest.preTest Any command(s) to run before test starts ["npm run script1", "npm run script2" (will run in the given order. If command(s) fail, tests will not run) []
vscode-vitest.postTest Any command(s) to run after test finishes ["npm run clean1", "npm run clean2" (will run in the given order) []
vscode-vitest.packageManager Desired package manager npm, yarn, pnpm, pnpn dlx, etc npx
vscode-vitest.testCommand Define an alternative vitest command test (e.g. for CRA, package.json test script, or similar abstractions) vitest
vscode-vitest.extraArguments Any additional vitest arguments --silent=true --maxWorkers=2 ""
vscode-vitest.executableOptions Executable option to show {"debug": false, "run": false} (will only display watch) {"debug": true,"run": true, "run": true}

Example Settings:

  "vscode-vitest.preTest": ["npm run script1"],
  "vscode-vitest.postTest": ["npm run cleanAfterTest"],
  "vscode-vitest.packageManager": "pnpm",
  "vscode-vitest.testCommand": "test",
  "vscode-vitest.extraArguments": "--silent --maxWorkers=2",
  "vscode-vitest.executableOptions": {
    "debug": false,
    "watch": false
  },

…', 'extraArguments', and 'executableOptions')
@cafesanu
Copy link
Contributor Author

Hey @rluvaton. Wondering if you had the chance to see this PR. We would love to use this extension with these changes

@rluvaton
Copy link
Owner

rluvaton commented Mar 13, 2024

Hey, sorry, looking now thanks for the contributions

package.json Outdated
Comment on lines 75 to 83
"vscode-vitest.executableOptions": {
"type": "array",
"items": {
"type": "string"
},
"default": ["run", "debug", "watch"],
"description": "Choose which executable options to enable"
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

the value should not be anything other than run, debug and watch, please change the items type to be enum instead of string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rluvaton
Copy link
Owner

I think we should the package manager option and instead have that in vscode-vitest.testCommand

so the default value will be npx vitest
and the user can replace to whatever they want to trigger vitest

src/run.ts Outdated
terminal = vscode.window.createTerminal(`vscode-vitest-runner`);
}
const pretest = (config.get("pretest") as string[]);
const finalPretest = pretest.length > 0 ? [`${pretest.join("; ")};`] : []
Copy link
Owner

Choose a reason for hiding this comment

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

  1. having ; would not fail if pretest command fail please replace to &&
  2. How other test runner extensions do this, this seems like can be security problem

Copy link
Contributor Author

@cafesanu cafesanu Mar 13, 2024

Choose a reason for hiding this comment

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

  1. Done
  2. The way jest runner does it is to simply change the jest command. So, regarding the security issue, I guess it's fine since they also allow you to that whatever and it's a pretty popular extension (1.000.000+ downloads) 🤷

This is what I currently have in mine (I'm using jest runner in the meanwhile while my changes take effect in this repo)
image

@rluvaton
Copy link
Owner

If I'm not answering don't hesitate and reach out on Twitter (rluvaton) as I have many notifications

@cafesanu
Copy link
Contributor Author

I think we should the package manager option and instead have that in vscode-vitest.testCommand

so the default value will be npx vitest and the user can replace to whatever they want to trigger vitest

@rluvaton I initally tried to do this, just like jest test runner does, but this line stopped me from doing it https://github.com/cafesanu/vitest-vs-code-plugin/blob/chore/support-settings/src/run.ts#L84 as it only needs the package manager, so I opted to separate it into two settings so I didn't have to refactor the whole thing

@cafesanu
Copy link
Contributor Author

cafesanu commented Mar 13, 2024

@rluvaton Lastly, I added a postTest option as well (3rd commit), as someone in my team needs this option and might be super useful in general

@cafesanu
Copy link
Contributor Author

@rluvaton sorry to keep bothering. Any chance you can review the requested changes. Would love to have these changes. Thanks!

@rluvaton
Copy link
Owner

Will get to it today

Copy link
Owner

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

Do you need to use pre and post scripts? I prefer to avoid due to security reasons

src/run.ts Outdated Show resolved Hide resolved
Co-authored-by: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>
@cafesanu
Copy link
Contributor Author

Do you need to use pre and post scripts? I prefer to avoid due to security reasons

Hey @rluvaton thanks for your review. My argument is that the most popular runner, jest runner, already lets you do this. In our case, we must run translations before we run unit test, and have to clean them after. My whole reason for creating this PR is that we need these steps.

But I totally understand your concern and don't want to force anything here. I would have to fork this extension and create another extension and publish it. Happy to do that if you feel uncomfortable with these changes

@rluvaton
Copy link
Owner

I'll test it manually tomorrow night and if everything ok I'll merge it, thank you for your patience

@rluvaton
Copy link
Owner

You had a small bug, fixed it and merging now, thank you!

@rluvaton rluvaton merged commit b0ea528 into rluvaton:main Mar 24, 2024
@rluvaton
Copy link
Owner

Published!

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