Converting TypeMoq tests to sinon #20274
Conversation
PR Changes
|
Benjin
requested review from
kburtram,
aasimkhan30,
caohai,
laurenastrid1,
lewis-sanchez,
allancascante,
ssreerama and
manujoseph85
as code owners
| * Avoid unnecessary casts, like `myVar as unknown as MyType` when myVar is already a sinon-stubbed instance of MyType. | ||
| * Maintain existing formatting conventions, line endings, and text encoding. | ||
| * Nest test suits as necessary to group tests in a logical manner. | ||
| - Do not edit application/source files unless the refactor demands it. Confirm before editing files outside of /test/unit, and justify why you need to make those changes. |
There was a problem hiding this comment.
all .md formatting is a side-effect of the new prettier rules. #20275 addresses those separately, and will be pulled into this PR once merged.
| searchService = new DatabaseObjectSearchService(client.object); | ||
| sandbox = sinon.createSandbox(); | ||
| client = sandbox.createStubInstance(SqlToolsServiceClient); | ||
| searchService = new DatabaseObjectSearchService(client as unknown as SqlToolsServiceClient); |
There was a problem hiding this comment.
Is this the kind of casting you were trying to avoid or is this fine?
There was a problem hiding this comment.
Exactly the kind I was hoping to avoid with the prompts. Not all of them can be eliminated (without some type-system sorcery), but thanks for calling out the ones that have snuck through so I can double-check them!
| appendLine: stubber.stub(), | ||
| clear: stubber.stub(), | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| show: stubber.stub() as any, |
There was a problem hiding this comment.
Why did you need to cast this to any, but not for the others?
There was a problem hiding this comment.
I wasn't able to get rid of this any because show() has several overloads that are confusing sinon's extraction of the valid types through the type system. If OutputChannel was a class, I could just stub that directly, but that isn't an option for interfaces.
| return { | ||
| extensionUri: vscode.Uri.parse("file://test"), | ||
| extensionPath: "path", | ||
| } as unknown as vscode.ExtensionContext; |
There was a problem hiding this comment.
Why did you need to cast to unknown before casting to a specific type?
| }); | ||
|
|
||
| suite("readProfileFromArgs", () => { | ||
| test("Should include connection properties that aren't sourced from SQL Tools Service", async () => { |
There was a problem hiding this comment.
Were these removed tests moved somewhere else or do we not want them anymore?
| expect(openConnectionDialogStub).to.not.have.been.called; | ||
| }); | ||
|
|
||
| test("Should ignore partially-matching profile when additional specifiers do not match", async () => { |
There was a problem hiding this comment.
Did we intentionally remove this test or something Copilot did while making changes?
| runnerMock.setHasCompleted(); | ||
| }); | ||
|
|
||
| runner = runnerMock as unknown as QueryRunner; |
There was a problem hiding this comment.
Same question as before about casting to unknown as an intermediary cast. Just commenting in case this needs to change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20274 +/- ##
==========================================
+ Coverage 54.34% 54.42% +0.07%
==========================================
Files 196 196
Lines 17526 17526
Branches 1165 1163 -2
==========================================
+ Hits 9525 9538 +13
+ Misses 8001 7988 -13 🚀 New features to boost your workflow:
|
First batch of TypeMoq -> Sinon test conversions, using AI with a good bit of coaching.
I've rolled most of the prompt/rules I've been using into
test/unit/AGENTS.md.Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines