Skip to content
This repository was archived by the owner on Feb 28, 2026. It is now read-only.

Fix and enable core unit test coverage#18781

Merged
Charles-Gagnon merged 5 commits intomainfrom
chgagnon/fixCoverage
Mar 21, 2022
Merged

Fix and enable core unit test coverage#18781
Charles-Gagnon merged 5 commits intomainfrom
chgagnon/fixCoverage

Conversation

@Charles-Gagnon
Copy link
Contributor

@Charles-Gagnon Charles-Gagnon commented Mar 18, 2022

I was able to track down what was causing the coverage to hang, but in the end I don't have a fully clear idea of why it's failing for us and presumably not for VS Code.

The line in question that led me to the fix was this :

[4225:0318/190144.273645:INFO:CONSOLE(109)] "Uncaught SyntaxError: Unexpected token '??'", source: vm.js (109)

After digging around in loader.js for a bit I was able to get it to spit out what file it came from and from there was just a matter of seeing which one of the ??'s was causing the issue.

There are numerous other reports of similar invalid code generation being done by instanbul, but since this fixes the issue and is relatively scoped I chose to end my investigation here since there isn't much value to be gained currently from digging into it more.

https://coveralls.io/builds/47556601

image

@coveralls
Copy link

coveralls commented Mar 18, 2022

Pull Request Test Coverage Report for Build 2018765633

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 113 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-5.5%) to 42.51%

Files with Coverage Reduction New Missed Lines %
extensions/dacpac/src/wizard/dataTierApplicationWizard.ts 18 80.99%
extensions/sql-bindings/src/services/azureFunctionsService.ts 21 12.35%
extensions/sql-bindings/src/common/azureFunctionsUtils.ts 74 26.39%
Totals Coverage Status
Change from base Build 2006236141: -5.5%
Covered Lines: 27559
Relevant Lines: 60570

💛 - Coveralls

@chlafreniere
Copy link
Contributor

Can we map so that we're not comparing the out directory but instead the sql directory?

@Charles-Gagnon
Copy link
Contributor Author

I'll take a look at it if I have time (but not going to prioritize it unless we have a specific need for that asap), but first order of business is to at least get these up and running again.

@Charles-Gagnon
Copy link
Contributor Author

@chlafreniere Ok, this should fix generating coverage for the ts files now. The line commented out was added as part of the last merge - I'm going to follow up with VS Code to see if I can understand why it was added since there isn't any clues in the commit and I'm not sure why this would need to be done.

microsoft/vscode@4355270

This should be safe to do since it's only affecting local build - product builds weren't affected by this to begin with.

@Charles-Gagnon
Copy link
Contributor Author

Filed issue in VS Code repo as well since this happens in their repo as well : microsoft/vscode#145598

@Charles-Gagnon
Copy link
Contributor Author

@chlafreniere Thoughts on the marked.js file showing up separately? Since it's not compiled I'm not sure there's a way to map it back to the one under src/.

So we could either leave it as it is or just remove it from the report completely.

@chlafreniere
Copy link
Contributor

@chlafreniere Thoughts on the marked.js file showing up separately? Since it's not compiled I'm not sure there's a way to map it back to the one under src/.

So we could either leave it as it is or just remove it from the report completely.

Personally, I'd vote for just removing it. Not convinced the value add (which itself is probably not very high -- it's not like we've made lots of changes in this file) is worth it enough to have this confusion over why the "out" folder shows up for just one file.

@Charles-Gagnon Charles-Gagnon merged commit 08b2d96 into main Mar 21, 2022
@Charles-Gagnon Charles-Gagnon deleted the chgagnon/fixCoverage branch March 21, 2022 22:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants