Skip to content

feat: add test for onLine throw error #4542

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.

Sign up for GitHub

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

repo-ranger merged 1 commit into main from jsjoeio-add-test-online

Nov 22, 2021

Merged

feat: add test for onLine throw error #4542

repo-ranger merged 1 commit into main from jsjoeio-add-test-online

Nov 22, 2021

Conversation

jsjoeio

Copy link

Contributor

@jsjoeio jsjoeio commented Nov 22, 2021 •

edited

Loading

This PR adds an additional test for the onLine util function to cover the scenario where the child.Process passed to the function is missing the stdout method. This adds code coverage for this line.

Screenshot

References

Fixes N/A
#TestingMondays

Blocked by #4543

@jsjoeio jsjoeio self-assigned this

@jsjoeio jsjoeio added the testing label

@jsjoeio jsjoeio added this to the 4.0.0 milestone

@github-actions

Copy link

github-actions bot commented Nov 22, 2021 •

edited

Loading

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

Host: https://codercom-flm35enk4-codercom.vercel.app/docs/code-server
Last deploy status: success
Commit: d4ba9bb
Workflow status: https://github.com/cdr/code-server/actions/runs/1491872274

Copy link

codecov bot commented Nov 22, 2021 •

edited

Loading

Codecov Report

Merging #4542 (5388efd) into main (0bc9698) will increase coverage by 0.06%.
The diff coverage is n/a.

❗ Current head 5388efd differs from pull request most recent head d4ba9bb. Consider uploading reports for the commit d4ba9bb to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@
## main #4542 +/- ##
==========================================
+ Coverage 65.38% 65.44% +0.06% 
==========================================
 Files 30 30 
 Lines 1664 1664 
 Branches 335 335 
==========================================
+ Hits 1088 1089 +1 
+ Misses 488 487 -1 
 Partials 88 88

Impacted Files	Coverage Δ
src/node/util.ts	`73.13% <0.00%> (+0.49%)`	⬆️

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 0bc9698...d4ba9bb. Read the comment docs.

jsjoeio commented

View reviewed changes

test/unit/node/util.test.ts Outdated

// @ts-expect-error TypeScript knows proc.stdout should return a Readable

// We overwrite it so that we can get testing coverage for the scenario

// in onLine that would throw an error.

proc.stdout = null

Copy link

Contributor Author

@jsjoeio jsjoeio Nov 22, 2021

Choose a reason for hiding this comment

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

According to the docs, it doesn't seem like I should have to do this but no matter what values I pass in the array to stdio it seems like proc.stdout is always set? I would love a second opinion here cc @vapurrmaid @bryphe-coder

Copy link

@bryphe-coder bryphe-coder Nov 22, 2021 •

edited

Loading

Choose a reason for hiding this comment

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

I think you might need different settings for cp.spawn to exercise this - checking

Copy link

@bryphe-coder bryphe-coder Nov 22, 2021

Choose a reason for hiding this comment

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

If I use the stdio flag 'ignore', it seems to work as you expect (in order to exercise the code under test):

const proc = cp.spawn("node", [], {
 stdio: 'ignore'
})

Copy link

@bryphe-coder bryphe-coder Nov 22, 2021

Choose a reason for hiding this comment

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

At least, tested this out in the NodeJS console:
image

Copy link

@bryphe-coder bryphe-coder Nov 22, 2021

Choose a reason for hiding this comment

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

Some useful docs around the various settings for options.stdio here: https://nodejs.org/api/child_process.html#optionsstdio

Copy link

Contributor Author

@jsjoeio jsjoeio Nov 22, 2021

Choose a reason for hiding this comment

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

Oh wow! Nice work 🙌 Okay I'll try that locally and see if it works for me as well.

Copy link

Contributor Author

@jsjoeio jsjoeio Nov 22, 2021

Choose a reason for hiding this comment

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

If I use the stdio flag 'ignore', it seems to work as you expect (in order to exercise the code under test):
const proc = cp.spawn("node", [], {
 stdio: 'ignore'
})

That worked!!!

Copy link

Contributor

@greyscaled greyscaled Nov 22, 2021

Choose a reason for hiding this comment

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

this one is better than my suggestion - glad it worked!

@jsjoeio jsjoeio marked this pull request as ready for review

November 22, 2021 19:14

@jsjoeio jsjoeio requested a review from a team as a code owner

November 22, 2021 19:14

bryphe-coder approved these changes

View reviewed changes

Copy link

@bryphe-coder bryphe-coder left a comment

Choose a reason for hiding this comment

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

Curious if make the change to use stdio: [null] -> stdio: 'ignore' works for you, in order to remove the manual setting of proc.stdout.

Otherwise, LGTM 👍

greyscaled reviewed

View reviewed changes

test/unit/node/util.test.ts Outdated

// which is why I have to set proc.stdout = null

// a couple lines below.

const proc = cp.spawn("node", [], {

stdio: [null],

Copy link

Contributor

@greyscaled greyscaled Nov 22, 2021

Choose a reason for hiding this comment

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

Does

stdio: [0, "empty"]

work?

Copy link

Contributor Author

@jsjoeio jsjoeio Nov 22, 2021

Choose a reason for hiding this comment

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

Looks like TS doesn't like this but stdio: 'ignore' worked!

Copy link

Contributor

@greyscaled greyscaled Nov 22, 2021

Choose a reason for hiding this comment

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

Nice!

greyscaled approved these changes

View reviewed changes

@jsjoeio jsjoeio force-pushed the jsjoeio-add-test-online branch from 6c3105a to afa5200 Compare

November 22, 2021 19:30


 feat: add test for onLine throw error

d4ba9bb

@jsjoeio jsjoeio force-pushed the jsjoeio-add-test-online branch from afa5200 to d4ba9bb Compare

November 22, 2021 19:51

@jsjoeio jsjoeio added the squash when passing label

@repo-ranger repo-ranger bot merged commit 65d7420 into main

@repo-ranger repo-ranger bot deleted the jsjoeio-add-test-online branch

November 22, 2021 20:19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 participants

@jsjoeio @greyscaled @bryphe-coder