-
-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor to use export map instead of paths in tests #49
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
Conversation
This closer resembles real world usage. It also tests that package exports are properly configured.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 512 512
=========================================
Hits 512 512 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ShowMessageRequest | ||
| } from 'vscode-languageserver-protocol/node.js' | ||
|
|
||
| test('exports', (t) => { |
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'm a bit cautious on removing tests.
There should still be export, even if they are partiallytested elsewhere?
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'm a bit cautious on removing tests.
I think that’s a good quality. :)
IMHO this test is a bit superfluous. It literally asserts the export is a function. All other tests test the behaviour of this function (fully, not partially).
Still, if you have strong feelings about this, I can restore it.
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 believe that the reason for something like this in the tests was because there was an export that was no longer needed.
A better way to test the API surface, which somewhat makes sure that we don’t export things we don’t need, and we do not accidentally remove things that we do need, would be something along the lines of:
const api = await import('unified-language-server')
const keys = Object.keys(api).sort()
assert.deepEqual(keys, ['something', 'somethingElse'])
This comment has been minimized.
This comment has been minimized.
|
Released in |
Initial checklist
Description of changes
This closer resembles real world usage. It also tests that package exports are properly configured.