Skip to content

Conversation

@abdala
Copy link
Contributor

@abdala abdala commented Mar 13, 2020

Usually in other test libraries we have available the method resolves(value) to be alias to returns(Promise.resolve(value))

substitute.returnPromise().returns(Promise.resolve(1338));

becomes:

substitute.returnPromise().resolves(1338);

References:

Copy link
Owner

@ffMathy ffMathy left a comment

Choose a reason for hiding this comment

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

Minor changes needed. Good idea.

type BaseMockObjectMixin<TReturnType> = {
returns: (...args: TReturnType[]) => void;
throws: (exception: any) => void;
resolves: (...args: Unpacked<TReturnType>[]) => void;
Copy link
Owner

Choose a reason for hiding this comment

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

resolves should only be available functions that return promises.

@ffMathy
Copy link
Owner

ffMathy commented Mar 13, 2020

Also, add documentation.

@abdala
Copy link
Contributor Author

abdala commented Mar 14, 2020

I'm not sure if this is the best implementation to restrict the method only for those that return a promise, but I'm open to suggestions.

@notanengineercom
Copy link
Collaborator

Really cool! Could you also implement in the same way rejects? With that we could have both promise results covered

@abdala abdala changed the title Add resolves() method Add resolves() and rejects() method Mar 15, 2020
Comment on lines 141 to 148
if (property === SubstituteMethods.resolves) {
returns = returns.map(value => Promise.resolve(value))
} else if(property === SubstituteMethods.rejects) {
returns = returns.map(value => Promise.reject(value))
}
this.returns.push({
returnValues: returns,
returnIndex: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice how you refactored it! But I don't think it's a good idea to reassigning variables, I think something like this would be better:

Suggested change
if (property === SubstituteMethods.resolves) {
returns = returns.map(value => Promise.resolve(value))
} else if(property === SubstituteMethods.rejects) {
returns = returns.map(value => Promise.reject(value))
}
this.returns.push({
returnValues: returns,
returnIndex: 0,
const returnMock: Partial<ReturnMock> = { returnIndex: 0, args: this._lastArgs };
switch (property) {
case SubstituteMethods.returns:
returnMock.returnValues = returns;
break;
case SubstituteMethods.resolves:
returnMock.returnValues = returns.map(value => Promise.resolve(value));
break;
case SubstituteMethods.rejects:
returnMock.returnValues = returns.map(value => Promise.reject(value));
break;
default:
throw SubstituteException.generic(
`Expected one of the following methods: "${SubstituteMethods.returns}", "${SubstituteMethods.resolves}" or "${SubstituteMethods.rejects}"`
);
}
this.returns.push(<ReturnMock>returnMock);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I just removed the default: throw from the code. It will never be reached in this case because of the if condition above :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, the if prevents the default case. I added it in the suggestion for the future, in case new methods are added and the switch case doesn't get updated. It makes it easier to debug

@ffMathy
Copy link
Owner

ffMathy commented Mar 16, 2020

If all tests pass, I'd say I have no more comments. Nicely done.

@notanengineercom when you are satisfied (and thanks for your review so far), feel free to merge.

@notanengineercom notanengineercom merged commit e25bf9d into ffMathy:master Mar 16, 2020
@abdala abdala deleted the promise-shortcut branch March 16, 2020 12:33
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.

3 participants