Skip to content

Conversation

@code-asher
Copy link
Member

@code-asher code-asher commented May 4, 2021

This is a workaround for #3014 until we figure it out (if it is indeed related which it might not be; the zlib errors could merely be side effects of a connection issue).

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #3286 (9afefcb) into main (75e9e24) will increase coverage by 2.76%.
The diff coverage is 37.23%.

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

@@            Coverage Diff             @@
##             main    #3286      +/-   ##
==========================================
+ Coverage   54.78%   57.54%   +2.76%     
==========================================
  Files          23       24       +1     
  Lines        1265     1279      +14     
  Branches      286      290       +4     
==========================================
+ Hits          693      736      +43     
+ Misses        460      441      -19     
+ Partials      112      102      -10     
Impacted Files Coverage Δ
src/node/entry.ts 0.00% <0.00%> (ø)
src/node/main.ts 36.78% <36.78%> (ø)
src/node/cli.ts 77.87% <100.00%> (+0.18%) ⬆️
src/node/constants.ts 100.00% <100.00%> (ø)
src/node/util.ts 48.97% <0.00%> (+2.04%) ⬆️
src/node/coder_cloud.ts 28.57% <0.00%> (+28.57%) ⬆️

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 75e9e24...a882be5. Read the comment docs.

@code-asher code-asher marked this pull request as ready for review May 4, 2021 21:57
@code-asher code-asher requested a review from a team as a code owner May 4, 2021 21:57
@jsjoeio
Copy link
Contributor

jsjoeio commented May 4, 2021

increase coverage by 0.12%

👏 You + Codecov helping us build a habit of increasing code coverage!

@jsjoeio jsjoeio added the enhancement Some improvement that isn't a feature label May 4, 2021
@jsjoeio jsjoeio added this to the v3.9.4 milestone May 4, 2021
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Overall, great work! 🎉 Just a couple nits about functions being pure

@code-asher code-asher force-pushed the permessage-deflate branch from 2889725 to 6ff495e Compare May 5, 2021 15:33
@code-asher code-asher marked this pull request as draft May 5, 2021 15:40
@code-asher code-asher force-pushed the permessage-deflate branch 3 times, most recently from 1da4d67 to fd4b784 Compare May 5, 2021 16:57
code-asher added 4 commits May 5, 2021 12:24
This is so they can be unit tested.
All it does is log and exit which is what the caller will be doing on an
error anyway (see entry).
@code-asher code-asher force-pushed the permessage-deflate branch from e06725d to a882be5 Compare May 5, 2021 17:24
@code-asher
Copy link
Member Author

Refactored; it's not any more pure but it's simpler. I think we could have an interesting discussion on whether we should pass args (and other variables) around instead of using a global access pattern at some point.

Coverage went down because of the code added in entry so I split some of those functions out in order to test them. One of them is now used for all the integration tests so those should reflect reality more closely now.

@code-asher code-asher marked this pull request as ready for review May 5, 2021 17:43
@code-asher code-asher requested a review from jsjoeio May 5, 2021 17:43
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Great work 👏


it("/healthz", async () => {
;[, , codeServer] = await integration.setup(["--auth=none"], "")
codeServer = await integration.setup(["--auth=none"], "")
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks way cleaner/easier to read 👏

Copy link

Choose a reason for hiding this comment

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

👍 yea, the code before was 🤨 kinda inscrutable, but then again, I'm not a JavaScript/TypeScript person

@jsjoeio
Copy link
Contributor

jsjoeio commented May 5, 2021

I think we could have an interesting discussion on whether we should pass args (and other variables) around instead of using a global access pattern at some point

Agreed! I prefer passing args because it's easier to mock/test things. But global access does come in handy when writing code. It would be a good discussion though.

@jsjoeio
Copy link
Contributor

jsjoeio commented May 5, 2021

I'm going to fix the trivy scan in a PR soon so you can ignore for now

@code-asher code-asher merged commit af5a1c9 into coder:main May 5, 2021
@code-asher code-asher deleted the permessage-deflate branch May 5, 2021 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Some improvement that isn't a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants