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

Mock Uri in Jest with vscode-uri #245

Merged
merged 10 commits into from
Apr 7, 2021
Merged

Mock Uri in Jest with vscode-uri #245

merged 10 commits into from
Apr 7, 2021

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Apr 6, 2021

This PR is a small slice of the... "ambitious" previous size, which breaks much fewer things.

Effectively it's just the test changes from #218, using the actual Uri library vscode uses in our test mocks. It's not the exact same, but it's the best we're going to get and we can tweak it if needed.
This change includes fixes to some of our existing tests that broke since our previous implementation of mocked Uri was just a dummy function that returned a string.

My intention is to add this one change which we all agree is good, then reimplement the other change we consider good in separating the command definitions out to another file.

Reworking all the commands to use Config directly may still be on the table as we add more and more options, but there's an argument for the shell call commands to have explicit args not tied to Config. I'll consider this when implementing a solution in a later PR.

Without using this data in multiple places, the wrapper module didn't serve much
purpose and hampered code readability a bit.
@rogermparent rogermparent changed the title Combine Reader and CLI into One Module and Refactor Mock Uri in Jest with vscode-uri Apr 7, 2021
@rogermparent rogermparent marked this pull request as ready for review April 7, 2021 21:20
@rogermparent rogermparent requested a review from mattseddon April 7, 2021 21:20
Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

some minor feedback / questions

extension/src/git.test.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
extension/src/test/util.ts Outdated Show resolved Hide resolved
@rogermparent rogermparent merged commit 257d87d into master Apr 7, 2021
@rogermparent rogermparent deleted the nullary-exp-commands branch April 7, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants