feat: add more tests for browser/pages/vscode #3743
Conversation
jsjoeio
force-pushed
the
jsjoeio-add-tests-vscode
branch
from
3bd53d0 to
bac4716
Compare
This refactors some logic in src/browser/pages/vscode.ts related to setting the background color of the body to the editor background theme color.
jsjoeio
force-pushed
the
jsjoeio-add-tests-vscode
branch
from
bac4716 to
66dc4cc
Compare
Codecov Report
@@ Coverage Diff @@
## main #3743 +/- ##
==========================================
+ Coverage 61.55% 62.01% +0.46%
==========================================
Files 35 35
Lines 1813 1835 +22
Branches 365 370 +5
==========================================
+ Hits 1116 1138 +22
Misses 588 588
Partials 109 109
Continue to review full report at Codecov.
|
| @@ -95,8 +95,64 @@ try { | |||
| console.error(error) | |||
| } | |||
|
|
|||
| export function setBodyBackgroundToThemeBackgroundColor(document: Document, localStorage: Storage) { | |||
There was a problem hiding this comment.
At first, I didn't like the function signature — passing in document and localStorage — but then I justified it because:
- this function is only called in one place
- it makes it a lot easier to test when
documentorlocalStorageis missing
There was a problem hiding this comment.
makes sense to me, this also allows you to test this by mocking document/localStorage, and avoids global state
| ) | ||
| } | ||
|
|
||
| document.body.style.background = editorBgColor |
There was a problem hiding this comment.
Here we assume that body is part of the document but I think it would be really rare/unusual if this weren't the case. I think we can leave as is and if we run into the rare issue, I can modify down the road.
| } catch (error) { | ||
| // Oh well. | ||
| console.error("Something went wrong setting the body background to the theme background color.") | ||
| console.error(error) |
There was a problem hiding this comment.
setBodyBackgroundToThemeBackgroundColor throws specific errors, which will be caught by our catch block and logged to the console in the browser.
| @@ -95,8 +95,64 @@ try { | |||
| console.error(error) | |||
| } | |||
|
|
|||
| export function setBodyBackgroundToThemeBackgroundColor(document: Document, localStorage: Storage) { | |||
There was a problem hiding this comment.
makes sense to me, this also allows you to test this by mocking document/localStorage, and avoids global state
| // We need to set the url in the JSDOM constructor | ||
| // to prevent this error "SecurityError: localStorage is not available for opaque origins" | ||
| // See: https://github.com/jsdom/jsdom/issues/2304#issuecomment-622314949 | ||
| const { window } = new JSDOM("", { url: "http://localhost" }) |
There was a problem hiding this comment.
I guess this is because Node.js doesn't have window?
There was a problem hiding this comment.
Exactly! window is only available in browser environments because it's a browser global
This PR adds more tests for
browser/pages/vscode.ts.Fixes N/A
Notes
The main purpose of this PR is to test this piece of code
This code grabs the
colorThemeDatafromlocalStorageto get the editor background color based on the user's VSCode theme and then sets the background of thebodyto that color.