BREAKING CHANGE: rename unitTestUtils and alter its behavior

[dpopp07]

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 a devDependency, and for linting reasons, lived in the test/ 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 another expect 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.

[MasterOdin]

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. * or >= 25 or something of that nature). Given how stable the API of expect is at this point, it's probably fairly safe to do the latter and not worry about breaking changes to how toEqual works.

[padamstx]

... (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? :)

[padamstx]

Ummm, I think we have a copy/paste error here :)

[dpopp07]

Dang... I put those exclamation marks to remind myself to fix it. Usually I remember to check 😅

[padamstx]

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.

[dpopp07]

@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.

Ok, I detected an attempt at a pun here. Do I win a prize? :)

It seems that you noticed that before I did 😆 We'll have to see about the prize

[germanattanasio]

LGTM

[dpopp07]

@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 toEqual and toBe and the methods in the class are pretty stable as well. This would add "type checking" for TS and vanilla JS without any dependencies.

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.

[MasterOdin]

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 typeof ExpectType in Options and SdkUnitTestUtilities. You would then just need to include @types/jest as a dependency (which it already is) and I think it would all just work with typing as expected, but would not bring in jest or expect as dependencies.

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?

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.

[dpopp07]

@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 expect as a package and use it in that file. We wouldn't have to create a class or use dependency injection. Although I like the class, we wouldn't need a breaking change that way. We can move everything to lib/ and export it the same way. This would remove all of the problematic "including test code/dependencies in the published package" stuff. We would just have a package as a dependency that you would normally see as a dev dependency, which might not be such a big deal.

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

[MasterOdin]

We would just have a package as a dependency that you would normally see as a dev dependency, which might not be such a big deal.

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 expect would be the thing that broke the camel's back.

[dpopp07]

@MasterOdin Sounds good, that's the approach I'm going to take. Thank you for weighing in on this! I appreciate it

[ibm-devx-automation]

🎉 This PR is included in version 2.4.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀