Skip to content

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

Merged
jsjoeio merged 14 commits into
mainfrom
4913-testing-test-handleargssocketcatcherror-is-called-on-unlink-error
Mar 11, 2022
Merged

feat(testing): add test for app > listen#4971
jsjoeio merged 14 commits into
mainfrom
4913-testing-test-handleargssocketcatcherror-is-called-on-unlink-error

Conversation

@jsjoeio

@jsjoeio jsjoeio commented Mar 9, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread test/unit/node/app.test.ts Outdated
@github-actions

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

Copy link
Copy Markdown

✨ 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

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

Copy link
Copy Markdown
Member

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

jsjoeio commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

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

codecov Bot commented Mar 10, 2022 •
edited
Loading

Copy link
Copy Markdown

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
Comment thread src/node/app.ts Outdated
Comment thread src/node/app.ts Outdated
Comment thread test/unit/node/constants.test.ts
Comment thread test/unit/node/constants.test.ts
@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
Comment thread src/node/app.ts Outdated
@jsjoeio
Update src/node/app.ts
1329a76 
Co-authored-by: Jonathan Yu <jonathan@coder.com>
code-asher
code-asher reviewed Mar 11, 2022
View reviewed changes
Comment thread src/node/app.ts Outdated
@jsjoeio jsjoeio temporarily deployed to npm March 11, 2022 20:21 Inactive
@jsjoeio @code-asher
Update src/node/app.ts
756ec12 
Co-authored-by: Asher <ash@coder.com>
@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 @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 <jonathan@coder.com>

* Update src/node/app.ts

Co-authored-by: Asher <ash@coder.com>

Co-authored-by: Jonathan Yu <jonathan@coder.com>
Co-authored-by: Asher <ash@coder.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@code-asher code-asher code-asher approved these changes
+1 more reviewer
@jawnsy jawnsy jawnsy left review comments
Reviewers whose approvals may not affect merge requirements

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