feat: add tests for shouldRunVsCodeCli and bindAddrFromArgs #4211

jsjoeio · 2021-09-20T23:09:27Z

This PR adds more tests for src/node/cli.ts. Specifically, these functions: shouldRunVsCodeCli and bindAddrFromArgs

Fixes #4210

TODOS

shouldRunVsCodeCli
bindAddrFromArgs
fix broken tests? (it seems one test is broken locally for me, probably a local issue)

codecov · 2021-09-20T23:11:14Z

Codecov Report

Merging #4211 (d921829) into main (92d0d28) will increase coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head d921829 differs from pull request most recent head f847575. Consider uploading reports for the commit f847575 to get more accurate results

@@            Coverage Diff             @@
##             main    #4211      +/-   ##
==========================================
+ Coverage   64.94%   65.09%   +0.14%     
==========================================
  Files          36       36              
  Lines        1880     1882       +2     
  Branches      381      380       -1     
==========================================
+ Hits         1221     1225       +4     
+ Misses        560      559       -1     
+ Partials       99       98       -1

Impacted Files	Coverage Δ
src/node/cli.ts	`80.99% <100.00%> (+0.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92d0d28...f847575. Read the comment docs.

github-actions · 2021-09-20T23:17:27Z

✨ Coder.com for PR #4211 deployed! It will be updated on every commit.

Host: https://codercom-32w99m3vb-codercom.vercel.app/docs/code-server
Last deploy status: success
Commit: f847575
Workflow status: https://github.com/cdr/code-server/actions/runs/1258886985

jsjoeio · 2021-09-20T23:17:45Z

I'm a bit dumbfounded. The tests pass in CI: https://github.com/cdr/code-server/pull/4211/checks?check_run_id=3657430012

But 1 test is not passing locally 🤔 It's the should catch errors thrown when unlinking a socket test.

 it("should catch errors thrown when unlinking a socket", async () => {
    const tmpDir2 = await tmpdir("unlink-socket-error")
    const tmpFile = path.join(tmpDir2, "unlink-socket-file")
    // await promises.writeFile(tmpFile, "")
    const socketPath = tmpFile
    const defaultArgs = await setDefaults({
      _: [],
      socket: socketPath,
    })

    const app = await createApp(defaultArgs)
    const server = app[2]

    expect(spy).toHaveBeenCalledTimes(1)
    expect(spy).toHaveBeenCalledWith(`ENOENT: no such file or directory, unlink '${socketPath}'`)

    server.close()
    // Ensure directory was removed
    rmdirSync(tmpDir2, { recursive: true })
  })

Basically, what's happening is:

I pass in a path to a file that doesn't exist for socket
It tries to unlink the file
Then it should call the spy (which is our logger), to log the error

BUT it does all this but then gets to server.listen and instead throws and error saying "address already in use:

See below:

      try {
        await fs.unlink(args.socket)
      } catch (error: any) {
        handleArgsSocketCatchError(error)
      }
      server.listen(args.socket, resolve

jsjoeio · 2021-09-20T23:17:59Z

help @code-asher - any ideas?

jsjoeio · 2021-09-21T17:42:46Z

src/node/cli.ts

+ * This function creates the bind address
+ * using the CLI args.
+ */
+export function bindAddrFromArgs(addr: Addr, args: Args): Addr {


🛠️ Exported so that we can unit test. Added short description for good measure.

jsjoeio · 2021-09-21T17:46:14Z

src/node/cli.ts

-  return !!args["list-extensions"] || !!args["install-extension"] || !!args["uninstall-extension"]
+  // Create new interface with only these keys
+  // Pick<Args, "list-extensions" | "install-extension" | "uninstall-extension">
+  // Get the keys of new interface
+  // keyof ...
+  // Turn that into an array
+  // Array<...>
+  type ExtensionArgs = Array<keyof Pick<Args, "list-extensions" | "install-extension" | "uninstall-extension">>
+  const extensionRelatedArgs: ExtensionArgs = ["list-extensions", "install-extension", "uninstall-extension"]
+
+  const argKeys = Object.keys(args)
+
+  // If any of the extensionRelatedArgs are included in args
+  // then we don't want to run the vscode cli
+  return extensionRelatedArgs.some((arg) => argKeys.includes(arg))


🔁 Refactored this function.

"But isn't this code more verbose?"

"Yes, but here's why I think it's an improvement:

!! is a clever technique in JS to cast a value to a Boolean, but it's hard to read.

previously these were hard-coded values, which means they weren't tied to the Args type they're based off

Now we have:

clear comments explaining what's happening

type-safety because the args we check for are now based on the Args type so if those change, TS will complain and we'll catch that here"

Happy to revert the change if we think this function should be left as it was!

bryphe-coder · 2021-09-21T18:37:24Z

src/node/cli.ts

+  // keyof ...
+  // Turn that into an array
+  // Array<...>
+  type ExtensionArgs = Array<keyof Pick<Args, "list-extensions" | "install-extension" | "uninstall-extension">>


I like the overall strategy to make this type-safe and get the benefit from the compiler! The only thing I find a bit awkward is that we have to reference the "list-extensions" | "install-extensions" | "uninstall-extensions" twice here.

I wonder if we could get most of the benefits, without having to define these multiple times, by doing:

type ArgumentKeys = Array<keyof Args>; const extensionRelatedArgs: ArgumentKeys = ["list-extensions", "install-extension", "uninstall-extension"];

That way - the compiler will still flag an error if these get out-of-sync, but you only have to define them once here in shouldRunVsCodeCli

Oh this is a huge improvement. Waaaaay better. Didn't think about that. Thanks for providing this suggestion! I'll incorporate this suggestion before merging! 🙌

bryphe-coder · 2021-09-21T18:39:47Z

test/unit/node/cli.test.ts

+  })
+
+  it("should use the args.port over process.env.PORT if both set", () => {
+    const [setValue, resetValue] = useEnv("PORT")


Nice set of tests 👍

bryphe-coder

Looks great to me - nice work @jsjoeio ! I like your approach to leveraging the TS type system as much as possible.

Just had minor feedback about streamlining having to define the "list-extensions" | "install-extension" | "uninstall-extension". Up to you if you want to incorporate that or not 👍

greyscaled

✔️ Nice work!

jsjoeio added 2 commits September 20, 2021 14:53

feat: add tests for shouldRunVsCodeCli

30ade71

refactor: shouldRunVsCodeCli

a673cf2

jsjoeio self-assigned this Sep 20, 2021

jsjoeio added the testing Anything related to testing label Sep 20, 2021

jsjoeio changed the title ~~Jsjoeio-add-moar-tests~~ feat: add tests for shouldRunVsCodeCli and bindAddrFromArgs Sep 20, 2021

jsjoeio commented Sep 21, 2021

View reviewed changes

jsjoeio marked this pull request as ready for review September 21, 2021 17:47

jsjoeio requested a review from a team as a code owner September 21, 2021 17:47

bryphe-coder reviewed Sep 21, 2021

View reviewed changes

bryphe-coder approved these changes Sep 21, 2021

View reviewed changes

feat: add tests for bindAddrFromArgs

f847575

jsjoeio force-pushed the jsjoeio-add-moar-tests branch from 541b3cc to f847575 Compare September 21, 2021 18:48

jsjoeio enabled auto-merge September 21, 2021 18:51

jsjoeio merged commit e8063c7 into main Sep 21, 2021

jsjoeio deleted the jsjoeio-add-moar-tests branch September 21, 2021 18:55

greyscaled reviewed Sep 21, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tests for shouldRunVsCodeCli and bindAddrFromArgs #4211

feat: add tests for shouldRunVsCodeCli and bindAddrFromArgs #4211

jsjoeio commented Sep 20, 2021 •

edited

Loading

codecov bot commented Sep 20, 2021 •

edited

Loading

github-actions bot commented Sep 20, 2021 •

edited

Loading

jsjoeio commented Sep 20, 2021

jsjoeio commented Sep 20, 2021

jsjoeio Sep 21, 2021

bryphe-coder Sep 21, 2021

jsjoeio Sep 21, 2021

bryphe-coder Sep 21, 2021

jsjoeio Sep 21, 2021

bryphe-coder Sep 21, 2021

jsjoeio Sep 21, 2021

bryphe-coder left a comment

greyscaled left a comment

feat: add tests for shouldRunVsCodeCli and bindAddrFromArgs #4211

feat: add tests for shouldRunVsCodeCli and bindAddrFromArgs #4211

Conversation

jsjoeio commented Sep 20, 2021 • edited Loading

codecov bot commented Sep 20, 2021 • edited Loading

Codecov Report

github-actions bot commented Sep 20, 2021 • edited Loading

jsjoeio commented Sep 20, 2021

jsjoeio commented Sep 20, 2021

jsjoeio Sep 21, 2021

Choose a reason for hiding this comment

bryphe-coder Sep 21, 2021

Choose a reason for hiding this comment

jsjoeio Sep 21, 2021

Choose a reason for hiding this comment

bryphe-coder Sep 21, 2021

Choose a reason for hiding this comment

jsjoeio Sep 21, 2021

Choose a reason for hiding this comment

bryphe-coder Sep 21, 2021

Choose a reason for hiding this comment

jsjoeio Sep 21, 2021

Choose a reason for hiding this comment

bryphe-coder left a comment

Choose a reason for hiding this comment

greyscaled left a comment

Choose a reason for hiding this comment

jsjoeio commented Sep 20, 2021 •

edited

Loading

codecov bot commented Sep 20, 2021 •

edited

Loading

github-actions bot commented Sep 20, 2021 •

edited

Loading