Add argument validation for sportstg-api functions #1
Conversation
| test('Get ladder validates compId', () => { | ||
| expect.assertions(1) | ||
| return connector.getLadder() | ||
| .then(() => {}) //Should not return ladder object |
There was a problem hiding this comment.
Unless I'm mistaken, .then should never run in this case. I think it would be good to have a fail in the then block. (Does jest even have a fail()?)
There was a problem hiding this comment.
You're right, .then is a failure condition in these tests as we're testing that the validation actually throws an AssertionError when input is invalid.
Regarding these fail comments, I looked into what others have used to fail a test on a resolved promise, and there are a number of alternatives to what I've used, but nothing explicit such as a jest.fail() equivalent. See this issue and this one for examples.
As is these tests fail when the promise resolves, since no assertion is called as per expect.assertions(1), but we could just add a expect(true).toBe(false) if you'd like for the test to be more explicit. Let me know your thoughts.
EDIT: We could also just throw an Error object in the .then() path, again I'd like to go with a failure method that you'd like consistently used in the test suite.
There was a problem hiding this comment.
Actually, I've found this way: https://jestjs.io/docs/en/expect#rejects
test('rejects to octopus', async () => {
await expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus');
});Important note from the documentation:
If the promise is fulfilled the assertion fails.
So it looks like what we want in one line of Perl Javascript
| test('Get fixtures validates compId', () => { | ||
| expect.assertions(1) | ||
| return connector.getRoundFixtures() | ||
| .then(() => {}) //Should not return fixtures object |
There was a problem hiding this comment.
Same comment as before, .then should have a fail()!
| return connector.getRoundFixtures(compId) | ||
| .then(() => {}) //Should not return fixtures object | ||
| .catch((error) =>{ | ||
| expect(error).toBeInstanceOf(assert.AssertionError) |
There was a problem hiding this comment.
Same comment as before, .then should have a fail()!
| function validateArgs(){ | ||
| let args = Array.prototype.slice.call(arguments, 0) //Get provided arguments to function | ||
| for(let i = 0; i < args.length; i++){ | ||
| assert(args[i] !== undefined, 'Missing required argument') |
There was a problem hiding this comment.
It would improve the user experience if it showed in the error message what variable was undefined. I'll leave it to you to come up with a solution to that ;)
Also, might as well check for null as well!
There was a problem hiding this comment.
Both of these are good points, I'll add this functionality in my next commit.
This PR validates the required arguments given to getLadder and getRoundFixtures, to ensure invalid invocations of these functions do not return ambiguous results.
The motivation for this comes from the fact that currently, calling getRoundFixtures without passing roundNum will still return an empty list, which makes it unclear without reading the documentation (...) that anything has gone wrong. When contrasting this to the fact that getLadder accepts roundNum as an optional argument, the confusion further increases.