Skip to content

feat: add more tests for browser/pages/vscode #3743

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

Merged
merged 1 commit into from
Jul 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 58 additions & 2 deletions src/browser/pages/vscode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,64 @@ try {
console.error(error)
}

export function setBodyBackgroundToThemeBackgroundColor(document: Document, localStorage: Storage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 document or localStorage is missing

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me, this also allows you to test this by mocking document/localStorage, and avoids global state

const errorMsgPrefix = "[vscode]"

if (!document) {
throw new Error(`${errorMsgPrefix} Could not set body background to theme background color. Document is undefined.`)
}

if (!localStorage) {
throw new Error(
`${errorMsgPrefix} Could not set body background to theme background color. localStorage is undefined.`,
)
}

const colorThemeData = localStorage.getItem("colorThemeData")

if (!colorThemeData) {
throw new Error(
`${errorMsgPrefix} Could not set body background to theme background color. Could not find colorThemeData in localStorage.`,
)
}

let _colorThemeData
try {
// We wrap this JSON.parse logic in a try/catch
// because it can throw if the JSON is invalid.
// and instead of throwing a random error
// we can throw our own error, which will be more helpful
// to the end user.
_colorThemeData = JSON.parse(colorThemeData)
} catch {
throw new Error(
`${errorMsgPrefix} Could not set body background to theme background color. Could not parse colorThemeData from localStorage.`,
)
}

const hasColorMapProperty = Object.prototype.hasOwnProperty.call(_colorThemeData, "colorMap")
if (!hasColorMapProperty) {
throw new Error(
`${errorMsgPrefix} Could not set body background to theme background color. colorThemeData is missing colorMap.`,
)
}

const editorBgColor = _colorThemeData.colorMap["editor.background"]

if (!editorBgColor) {
throw new Error(
`${errorMsgPrefix} Could not set body background to theme background color. colorThemeData.colorMap["editor.background"] is undefined.`,
)
}

document.body.style.background = editorBgColor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


return null
}

try {
document.body.style.background = JSON.parse(localStorage.getItem("colorThemeData")!).colorMap["editor.background"]
setBodyBackgroundToThemeBackgroundColor(document, localStorage)
} catch (error) {
// Oh well.
console.error("Something went wrong setting the body background to the theme background color.")
console.error(error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

setBodyBackgroundToThemeBackgroundColor throws specific errors, which will be caught by our catch block and logged to the console in the browser.

}
119 changes: 118 additions & 1 deletion test/unit/browser/vscode.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
* @jest-environment jsdom
*/
import { JSDOM } from "jsdom"
import { getNlsConfiguration, nlsConfigElementId } from "../../../src/browser/pages/vscode"
import {
getNlsConfiguration,
nlsConfigElementId,
setBodyBackgroundToThemeBackgroundColor,
} from "../../../src/browser/pages/vscode"

describe("vscode", () => {
describe("getNlsConfiguration", () => {
Expand Down Expand Up @@ -58,4 +62,117 @@ describe("vscode", () => {
document.body.removeChild(mockElement)
})
})
describe("setBodyBackgroundToThemeBackgroundColor", () => {
beforeEach(() => {
// 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" })
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is because Node.js doesn't have window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly! window is only available in browser environments because it's a browser global

global.document = window.document
global.localStorage = window.localStorage
})
it("should return null", () => {
const test = {
colorMap: {
[`editor.background`]: "#ff3270",
},
}
localStorage.setItem("colorThemeData", JSON.stringify(test))

expect(setBodyBackgroundToThemeBackgroundColor(document, localStorage)).toBeNull()

localStorage.removeItem("colorThemeData")
})
it("should throw an error if Document is undefined", () => {
const errorMsgPrefix = "[vscode]"
const errorMessage = `${errorMsgPrefix} Could not set body background to theme background color. Document is undefined.`

expect(() => {
// @ts-expect-error We need to test when document is undefined
setBodyBackgroundToThemeBackgroundColor(undefined, localStorage)
}).toThrowError(errorMessage)
})
it("should throw an error if localStorage is undefined", () => {
const errorMsgPrefix = "[vscode]"
const errorMessage = `${errorMsgPrefix} Could not set body background to theme background color. localStorage is undefined.`

expect(() => {
// @ts-expect-error We need to test when localStorage is undefined
setBodyBackgroundToThemeBackgroundColor(document, undefined)
}).toThrowError(errorMessage)
})
it("should throw an error if it can't find colorThemeData in localStorage", () => {
const errorMsgPrefix = "[vscode]"
const errorMessage = `${errorMsgPrefix} Could not set body background to theme background color. Could not find colorThemeData in localStorage.`

expect(() => {
setBodyBackgroundToThemeBackgroundColor(document, localStorage)
}).toThrowError(errorMessage)
})
it("should throw an error if there is an error parsing colorThemeData from localStorage", () => {
const errorMsgPrefix = "[vscode]"
const errorMessage = `${errorMsgPrefix} Could not set body background to theme background color. Could not parse colorThemeData from localStorage.`

localStorage.setItem(
"colorThemeData",
'{"id":"vs-dark max-SS-Cyberpunk-themes-cyberpunk-umbra-color-theme-json","label":"Activate UMBRA protocol","settingsId":"Activate "errorForeground":"#ff3270","foreground":"#ffffff","sideBarTitle.foreground":"#bbbbbb"},"watch\\":::false}',
)

expect(() => {
setBodyBackgroundToThemeBackgroundColor(document, localStorage)
}).toThrowError(errorMessage)

localStorage.removeItem("colorThemeData")
})
it("should throw an error if there is no colorMap property", () => {
const errorMsgPrefix = "[vscode]"
const errorMessage = `${errorMsgPrefix} Could not set body background to theme background color. colorThemeData is missing colorMap.`

const test = {
id: "hey-joe",
}
localStorage.setItem("colorThemeData", JSON.stringify(test))

expect(() => {
setBodyBackgroundToThemeBackgroundColor(document, localStorage)
}).toThrowError(errorMessage)

localStorage.removeItem("colorThemeData")
})
it("should throw an error if there is no editor.background color", () => {
const errorMsgPrefix = "[vscode]"
const errorMessage = `${errorMsgPrefix} Could not set body background to theme background color. colorThemeData.colorMap["editor.background"] is undefined.`

const test = {
id: "hey-joe",
colorMap: {
editor: "#fff",
},
}
localStorage.setItem("colorThemeData", JSON.stringify(test))

expect(() => {
setBodyBackgroundToThemeBackgroundColor(document, localStorage)
}).toThrowError(errorMessage)

localStorage.removeItem("colorThemeData")
})
it("should set the body background to the editor background color", () => {
const test = {
colorMap: {
[`editor.background`]: "#ff3270",
},
}
localStorage.setItem("colorThemeData", JSON.stringify(test))

setBodyBackgroundToThemeBackgroundColor(document, localStorage)

// When the body.style.backgroundColor is set using hex
// it is converted to rgb
// which is why we use that in the assertion
expect(document.body.style.backgroundColor).toBe("rgb(255, 50, 112)")

localStorage.removeItem("colorThemeData")
})
})
})