- Notifications
You must be signed in to change notification settings - Fork928
chore: updated documentation link#1387
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMay 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1387 +/- ##==========================================- Coverage 66.97% 66.79% -0.19%========================================== Files 288 288 Lines 18857 18857 Branches 241 241 ==========================================- Hits 12630 12596 -34- Misses 4939 4963 +24- Partials 1288 1298 +10
Continue to review full report at Codecov.
|
| const env = process.env | ||
| beforeEach(() => { | ||
| jest.resetModules() |
BrunoQuaresmaMay 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wondering if we need this, I'm not seeing a module being mocked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've actually never mocked outprocess.env before so I'm definitely following some internet advice here 😃 but looking at the Jest docs, seems like each test file gets its own module registryby default. Trying to reset the cache with this line so we don't get any conflicts. I can test and see if it's not needed but it might be safer to leave it in. LMK what you think!
BrunoQuaresmaMay 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If it is not needed, I think we should remove it, but if you think it is needed, I'm good to keep it. I'm good with the decision you feel is the best 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure; we can always add it back in if we see conflicts.
| beforeEach(() => { | ||
| jest.resetModules() | ||
| process.env = { ...env } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since env is already receiving the process.env in line 14, I would guess we are doing this to clone the object, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's right - I don't want to mutateprocess.env IRL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If makes sense, I just would add a comment about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. I just had some minor questions.
* chore: updated documentation link* PR feedback
Uh oh!
There was an error while loading.Please reload this page.
Resolves#1037.
I didn't see any appropriate env variables being set on host so I assumed passing one in like
CODER_VERSION=v0.5.4 ./develop.shwould suffice. This seems to work fine, but let me know if I misunderstood scope!