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

Knobs for unit-testing extensions #10178

Merged
merged 7 commits into from
May 4, 2020
Merged

Knobs for unit-testing extensions #10178

merged 7 commits into from
May 4, 2020

Conversation

Benjin
Copy link
Contributor

@Benjin Benjin commented Apr 27, 2020

Converted test-extensions-unit.bat to javascript to add a small QoL improvement that I think people will find useful: specifying which extension(s) you want to run tests for instead of commenting out lines of a script.

In addition to existing behavior being mostly preserved (node test-extensions-unit.js defaults to running all unit tests), you can specify one or more:

node test-extensions-unit.js sql-database-projects
node test-extensions-unit.js sql-database-projects schema-compare

And through the power of modern scripting languages, it now has auto-generated help text (node test-extensions-unit.js help, or any time invalid args are passed), parameter validation, and it's now a single-line change to add new extensions.

@Benjin Benjin requested a review from udeeshagautam April 27, 2020 22:04
@anthonydresser
Copy link
Contributor

Why not write these scripts in javascript rather than powershell?

@aaomidi
Copy link
Contributor

aaomidi commented Apr 29, 2020

I agree with @anthonydresser, if we're going to rewrite these scripts I'd prefer if they were in JS/TS which is a language all of us should be familiar with rather than powershell.

@Benjin
Copy link
Contributor Author

Benjin commented Apr 30, 2020

I realized it would be easier to rewrite the script in something modern than to try to add what I wanted in batch, so I defaulted toward powershell since it's been the default scripting language in Windows for years and can be executed directly same as the old script, so it's the closest thing to a drop-in replacement. But I hadn't thought about the possibility of merging the two scripts into one platform-agnostic one... If we're looking do that with test-extensions-unit (as I understand we can since they're ADS rather than merged from VS Code), then yeah - javascript makes the most sense.

@github-actions
Copy link

github-actions bot commented May 1, 2020

Coverage Status

Coverage increased (+0.06%) to 32.702% when pulling be1c6ab on benjin/unitTestKnobs into 8a6e918 on master.

@anthonydresser
Copy link
Contributor

Really shouldn't use yargs, we don't list it as an explicit dependency, so it could be removed at any point, and we already use minimist else where.

@Benjin Benjin requested a review from aaomidi May 4, 2020 17:26
@aaomidi
Copy link
Contributor

aaomidi commented May 4, 2020

From my understanding this would just be a new dev dependency, right? In that case, if you don't want to use existing dependencies (especially since they've been deprecated) that's fine in my opinion.

Copy link
Contributor

@aaomidi aaomidi left a comment

Choose a reason for hiding this comment

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

I don't think the library you use here matters much.

Yargs seems to be a larger library with a larger dependency tree, but seems to do more of what we want to achieve here.

Optimist is deprecated, so it's probably not a good idea to build a new script using that.

But since this is a dev dependency, it really doesn't matter if we introduce a new dependency I think. So whatever helps achieve this goal is fine imho.

@Benjin Benjin merged commit 5498c1c into master May 4, 2020
@Benjin Benjin deleted the benjin/unitTestKnobs branch May 4, 2020 20:11
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.

3 participants