Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add registerRequireOnSelf on function #3760

Merged
merged 1 commit into from
Jul 21, 2021
Merged

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Jul 9, 2021

This adds a new function called registerRequireOnSelf to vscode.ts. We do this so that we can extract this logic into a function, which makes it easier to test.

Screenshot

Screen.Recording.2021-07-12.at.12.10.24.PM.mov

Fixes N/A

TODOs

@jsjoeio jsjoeio added the testing Anything related to testing label Jul 9, 2021
@jsjoeio jsjoeio self-assigned this Jul 9, 2021
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #3760 (14845f4) into main (670f0a1) will increase coverage by 0.50%.
The diff coverage is 92.10%.

❗ Current head 14845f4 differs from pull request most recent head 911cb03. Consider uploading reports for the commit 911cb03 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3760      +/-   ##
==========================================
+ Coverage   62.10%   62.60%   +0.50%     
==========================================
  Files          36       36              
  Lines        1863     1872       +9     
  Branches      378      379       +1     
==========================================
+ Hits         1157     1172      +15     
+ Misses        600      595       -5     
+ Partials      106      105       -1     
Impacted Files Coverage Δ
src/browser/pages/vscode.ts 83.82% <92.10%> (+12.63%) ⬆️

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 670f0a1...911cb03. Read the comment docs.

@jsjoeio jsjoeio added this to the 3.11.0 milestone Jul 12, 2021
@jsjoeio jsjoeio changed the title wip: add registerRequireOnSelf on function feat: add registerRequireOnSelf on function Jul 12, 2021
@jsjoeio jsjoeio force-pushed the jsjoeio-add-vscode-test branch 4 times, most recently from 65c4f29 to 7a26391 Compare July 12, 2021 19:15
@jsjoeio jsjoeio marked this pull request as ready for review July 12, 2021 19:15
@jsjoeio jsjoeio requested a review from a team as a code owner July 12, 2021 19:15
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
test/unit/browser/vscode.test.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio requested a review from code-asher July 12, 2021 22:11
@jsjoeio jsjoeio marked this pull request as draft July 13, 2021 18:02
@jsjoeio jsjoeio marked this pull request as ready for review July 13, 2021 18:26
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jul 13, 2021

@code-asher refactored! I hope I understood your suggestions. Feel free to nitpick/correct again if I didn't!

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jul 13, 2021

will decrease coverage by 0.67%.

Ah, that's super frustrating :( Before all the refactoring, code coverage had increased. And now that I removed a bunch, I guess it's gone down. The joy of code-coverage + testing!

@code-asher I'll mark as draft and add more tests, but do you mind reviewing still?

@jsjoeio jsjoeio marked this pull request as draft July 13, 2021 20:54
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jul 13, 2021

All in all, I think this will actually be a lot better than the first iteration.

src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
test/unit/browser/pages/vscode.test.ts Outdated Show resolved Hide resolved
test/unit/browser/pages/vscode.test.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio marked this pull request as ready for review July 19, 2021 21:08
@jsjoeio jsjoeio requested a review from code-asher July 19, 2021 21:08
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jul 19, 2021

Eventually when this gets to a good state and is approved and merged, the vscode file will be bulletproof 😎

@jsjoeio jsjoeio modified the milestones: 3.11.0, 3.12.0 Jul 19, 2021
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Show resolved Hide resolved
src/browser/pages/vscode.ts Show resolved Hide resolved
src/browser/pages/vscode.ts Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
test/unit/browser/pages/vscode.test.ts Outdated Show resolved Hide resolved
test/unit/browser/pages/vscode.test.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
src/browser/pages/vscode.ts Show resolved Hide resolved
src/browser/pages/vscode.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio enabled auto-merge July 21, 2021 22:31
@jsjoeio jsjoeio merged commit 7b8cd25 into main Jul 21, 2021
@jsjoeio jsjoeio deleted the jsjoeio-add-vscode-test branch July 21, 2021 22:48
@jsjoeio jsjoeio modified the milestones: 3.12.0, 3.11.1 Aug 6, 2021
@ahmadyahya11 ahmadyahya11 mentioned this pull request Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Anything related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants