feat(testing): add tests for src/browser/pages/vscode to hit 100% coverage #4055
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4055 +/- ##
=======================================
Coverage 63.51% 63.51%
=======================================
Files 36 36
Lines 1872 1872
Branches 379 379
=======================================
Hits 1189 1189
Misses 580 580
Partials 103 103
Continue to review full report at Codecov.
|
|
✨ Coder.com for PR #4055 deployed! It will be updated on every commit.
|
jsjoeio
changed the title
jsjoeio
force-pushed
the
jsjoeio-tests-vscode
branch
from
889d476 to
011e8bf
Compare
| @@ -24,13 +24,13 @@ type NlsConfiguration = { | |||
| * Helper function to create the path to the bundle | |||
| * for getNlsConfiguration. | |||
| */ | |||
| export function createBundlePath(_resolvedLanguagePackCoreLocation: string, bundle: string) { | |||
| export function createBundlePath(_resolvedLanguagePackCoreLocation: string | undefined, bundle: string) { | |||
There was a problem hiding this comment.
🧹 updated the signature to match the actual value passed in _resolvedLanguagePackCoreLocation's type is string | undefined
| // NOTE@jsjoeio - this comment was here before me | ||
| // Refers to operating systems that use a different path separator. | ||
| // Probably just Windows but we're not sure if "/" breaks on Windows | ||
| // so we'll leave it alone for now. | ||
| // FIXME: Only works if path separators are /. | ||
| return _resolvedLanguagePackCoreLocation + "/" + bundle.replace(/\//g, "!") + ".nls.json" | ||
| return (_resolvedLanguagePackCoreLocation || "") + "/" + bundle.replace(/\//g, "!") + ".nls.json" |
There was a problem hiding this comment.
🧹 Originally, we were using _resolvedLanguagePackCoreLocation or passing in an empty string. We moved the logic from outside the function into the function since the caller shouldn't have to change the argument functions if the value is undefined. Instead, the function should handle this.
| @@ -72,20 +72,22 @@ export function getNlsConfiguration(_document: Document, base: string) { | |||
|
|
|||
| type LoadBundleCallback = (_: undefined, result?: string) => void | |||
|
|
|||
| nlsConfig.loadBundle = (bundle: string, _language: string, cb: LoadBundleCallback): void => { | |||
| nlsConfig.loadBundle = async (bundle: string, _language: string, cb: LoadBundleCallback): Promise<void> => { | |||
There was a problem hiding this comment.
🧹 Originally, this function was not using async/await. I then realized we had the fetch call and the .then chain. I refactored this so it was easy to test. Now it's clear to the caller this function has asynchronous logic.
| if (result) { | ||
| return cb(undefined, result) | ||
| } | ||
| // FIXME: Only works if path separators are /. |
There was a problem hiding this comment.
🧹 Removed this comment because it's related to createBundlePath and explained in a comment near that function with more details.
| @@ -87,6 +119,13 @@ describe("vscode", () => { | |||
| const actual = createBundlePath(_resolvedLangaugePackCoreLocation, bundle) | |||
| expect(actual).toBe(expected) | |||
| }) | |||
| it("should return the correct path (even if _resolvedLangaugePackCoreLocation is undefined)", () => { | |||
There was a problem hiding this comment.
✅ This check was added after modifying the createBundlePath function signature.
jsjoeio
force-pushed
the
jsjoeio-tests-vscode
branch
from
011e8bf to
f4b0ab3
Compare
jsjoeio
force-pushed
the
jsjoeio-tests-vscode
branch
from
f4b0ab3 to
79b4e47
Compare
jsjoeio
changed the title
jsjoeio
changed the title
This PR adds full test coverage for
src/browser/pages/vscode.tsFixes #4052