-
Notifications
You must be signed in to change notification settings - Fork 25
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
BREAKING CHANGE: rename unitTestUtils
and alter its behavior
#101
Conversation
the module is moved into a new class, `SdkUnitTestUtilities`, that does rely on `jest` or any dev dependencies. users must import the class, instantiate a new instance, and pass jest's `expect` method to the constructor. all methods are the same but must be accessed through the class.
A neat approach and would definitely fix the immediate concerns of the linked issues. However, a con of the approach is the loss of type checking within this new helper class. I would somewhat suggest adding expect as a dependency and using it to provide types to the expect variable within the class, though this has the downside of you will probably want to update expect to be inline with the leading version of jest in other repos to allow for deduplication, or just use a broad version range (e.g. |
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.
... (they should technically work with another
expect
package using the same API, not that I would expect any users to do that).
Ok, I detected an attempt at a pun here. Do I win a prize? :)
lib/sdk-test-helpers.ts
Outdated
private expect: Function; | ||
|
||
/** | ||
* Create a new BasicAuthenticator instance. !!!!!!! |
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.
Ummm, I think we have a copy/paste error here :)
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.
Dang... I put those exclamation marks to remind myself to fix it. Usually I remember to check 😅
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'll repeat my question from slack here. I was wondering if there is a less fragile way to introduce the new class. Could we just add the new class, then change the Node generator to use it, but otherwise leave the old functions alone (perhaps declare them as deprecated) so that older generated code would continue to work even if the user needed to move up to a new version of the Node core for some other reason (another bug fix, etc.)? We could then remove the deprecated function at some point in the future.
@padamstx The issue with that is - I opened this PR to resolve a few outstanding problems. Leaving the methods in for compatibility would sort of defeat the purpose of making the change in the first place, as the problems would also remain.
It seems that you noticed that before I did 😆 We'll have to see about the prize |
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
@MasterOdin I hear what you're saying, that's a great point. I'm just not sure adding another dependency is worth the type checking for TS only. This is a little hacky, but what are your thoughts on adding a validate method (called in the constructor) to ensure the expect variable has the chained methods we use? Right now, it is only EDIT: I'm silly and am just realizing adding the dependency would eliminate the need to accept a parameter from the user altogether. That being said, I am still not sure about adding a dependency for this. |
Playing a bit more around with this, another potential option would be to update to Typescript 3.8+, and then add at the top: import type {expect as ExpectType} from '@jest/globals'; and then update the type definition to be
I had originally typed up a reply suggesting something along these lines, and then deleted it as I thought it was too hacky, and disliked the manual effort needed to update the validator whenever you want to use new types. |
@MasterOdin I like the idea of importing just the types, thank you for the suggestion. Thinking about it more though, I'm wondering if it wouldn't just be altogether simpler to depend on It's either that or importing the types. I might be overthinking this but I made a bad call on how I handled this before and I don't want to have to touch this code again haha. I welcome any further thoughts from anyone |
At the end of the day, I think it's probably the best solution, except for maybe splitting this off into its own package. However, given the general package explosion one sees for installing most things, I doubt |
8e8ab42
to
1fa6dfe
Compare
@MasterOdin Sounds good, that's the approach I'm going to take. Thank you for weighing in on this! I appreciate it |
1fa6dfe
to
e02592d
Compare
this removes any need to include test code in the published package and avoids mixing dev dependencies into the published dependencies also, update axios-cookie-support to use the latest release and add tough-cookie, as it is now a required peer dependency
e02592d
to
75f0902
Compare
## [2.4.2](v2.4.1...v2.4.2) (2020-07-22) ### Bug Fixes * move test utilities to lib/ and rely directly on `expect` ([#101](#101)) ([57ca4c2](57ca4c2))
🎉 This PR is included in version 2.4.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Bumps [sinon](https://github.com/sinonjs/sinon) from 9.2.2 to 9.2.3. - [Release notes](https://github.com/sinonjs/sinon/releases) - [Changelog](https://github.com/sinonjs/sinon/blob/master/CHANGELOG.md) - [Commits](sinonjs/sinon@v9.2.2...v9.2.3) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There have been a number of issues with how I implemented the unit test helpers in this package. They relied on
jest
, which should only ever be adevDependency
, and for linting reasons, lived in thetest/
folder. This is the basis of a few issues: #81, #99, and node-sdk-1047.I want the testing utilities in this package, so I rewrote them as class that the jest
expect
method is injected into. This eliminates any external dependencies and actually makes them a little more generic (they should technically work with anotherexpect
package using the same API, not that I would expect any users to do that).All methods are the same but must be accessed through the class.
This does change the API of this package, so it is a breaking change. It should be the only breaking change in v3.