Skip to content

feat(testing): add test for app > listen #4971

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

Jump to bottom
Merged
merged 14 commits into from
Mar 11, 2022
Merged

feat(testing): add test for app > listen #4971

merged 14 commits into from
Mar 11, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Mar 9, 2022

This PR adds a new test to handle test when fs.unlink throws inside listen.

Fixes #4913

@jsjoeio jsjoeio added the testing Anything related to testing label Mar 9, 2022
@jsjoeio jsjoeio requested a review from a team March 9, 2022 23:22
@jsjoeio jsjoeio self-assigned this Mar 9, 2022
jsjoeio
jsjoeio commented Mar 9, 2022
View reviewed changes
@github-actions
Copy link

github-actions bot commented Mar 9, 2022 •
edited
Loading

✨ code-server docs for PR #4971 is ready! It will be updated on every commit.

@jsjoeio jsjoeio temporarily deployed to CI March 9, 2022 23:23 Inactive
code-asher
code-asher previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Test looks great. I wonder if we should actually throw on any non-ENOENT error here instead of just logging because otherwise we get:

[2022-03-10T19:08:49.832Z] error EISDIR: illegal operation on a directory, unlink '/home/coder'
[2022-03-10T19:08:49.835Z] error listen EADDRINUSE: address already in use /home/coder

Which feels to me not quite correct, /home/coder is not already in use, it is just not valid because it is a directory.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Mar 10, 2022

Which feels to me not quite correct, /home/coder is not already in use, it is just not valid because it is a directory.

Great suggestion! I like correct better. I'll update the code and the test. (test first of course 😉 )

@jsjoeio jsjoeio requested a review from code-asher March 10, 2022 22:58
@codecov
Copy link

codecov bot commented Mar 10, 2022 •
edited
Loading

Codecov Report

Merging #4971 (356ac7a) into main (91cabbc) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4971      +/-   ##
==========================================
+ Coverage   71.45%   71.58%   +0.12%     
==========================================
  Files          29       29              
  Lines        1678     1675       -3     
  Branches      373      373              
==========================================
  Hits         1199     1199              
+ Misses        408      405       -3     
  Partials       71       71
Impacted Files Coverage Δ
src/node/app.ts 100.00% <100.00%> (+5.17%) ⬆️

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 91cabbc...356ac7a. Read the comment docs.

@jsjoeio jsjoeio temporarily deployed to npm March 10, 2022 23:12 Inactive
@jsjoeio jsjoeio temporarily deployed to npm March 10, 2022 23:53 Inactive
@jsjoeio jsjoeio dismissed code-asher’s stale review March 11, 2022 17:16

Major changes made, need a new approval

@jsjoeio jsjoeio temporarily deployed to npm March 11, 2022 17:25 Inactive
code-asher
code-asher reviewed Mar 11, 2022
View reviewed changes
@jsjoeio jsjoeio requested a review from code-asher March 11, 2022 19:34
@jsjoeio jsjoeio temporarily deployed to npm March 11, 2022 20:07 Inactive
jawnsy
jawnsy reviewed Mar 11, 2022
View reviewed changes
@jsjoeio @jawnsy
Update src/node/app.ts
1329a76 
Co-authored-by: Jonathan Yu <[email protected]>
code-asher
code-asher reviewed Mar 11, 2022
View reviewed changes
@jsjoeio jsjoeio temporarily deployed to npm March 11, 2022 20:21 Inactive
@jsjoeio jsjoeio temporarily deployed to npm March 11, 2022 20:31 Inactive
@jsjoeio jsjoeio temporarily deployed to npm March 11, 2022 21:06 Inactive
@jsjoeio jsjoeio merged commit 86c8590 into main Mar 11, 2022
@jsjoeio jsjoeio deleted the 4913-testing-test-handleargssocketcatcherror-is-called-on-unlink-error branch March 11, 2022 23:55
TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022
@jsjoeio @jawnsy
@code-asher @TinLe
feat(testing): add test for app > listen (coder#4971)
8b54ebe 
* feat(testing): add test for app > listen

* Update test/unit/node/app.test.ts

* refactor: modernize listen fn in app

* wip

* fix: update error message

* fixup: remove console.log

* fixup: use clearAllMocks once in beforeAll

* fix: move chmod after socket listen

* fixup: formatting

* Update src/node/app.ts

Co-authored-by: Jonathan Yu <[email protected]>

* Update src/node/app.ts

Co-authored-by: Asher <[email protected]>

Co-authored-by: Jonathan Yu <[email protected]>
Co-authored-by: Asher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jawnsy jawnsy jawnsy left review comments

@code-asher code-asher code-asher approved these changes

Assignees

@jsjoeio jsjoeio

Labels
testing Anything related to testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Testing]: test handleArgsSocketCatchError is called on unlink error
3 participants
@jsjoeio @jawnsy @code-asher