-
Notifications
You must be signed in to change notification settings - Fork 1
Tests and things #1
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
base: master
Are you sure you want to change the base?
Conversation
- Extract spotify javascript and API calls into separate module - Add tests that correct calls are made with correct arguments
| @@ -0,0 +1,60 @@ | |||
| /* global Spotify */ | |||
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.
Isolate API and SDK side-effects for easier testing
| const original = axios.defaults.adapter | ||
| axios.defaults.adapter = request | ||
|
|
||
| const configWith = options => ({ |
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.
Not a huge fan of this - axios has a handful of default request configuration options which made it difficult to do a direct assertion on what the config looked like as a request.
Now that I'm looking at this, we could just record a collection of the config objects, which would make it easier to do something like expect(config[0].url).toBe('...') rather than abuse jest mock functions.
| ) | ||
| this.player.addListener('ready', ({device_id}) => { | ||
| this.player.on('ready', ({device_id}) => { |
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.
Looks like addListener and on do the same thing, so I switch this to make my test mocking easier, and to make this block more consistent.
| } | ||
| }); | ||
| SpotifyService.shuffle(deviceId, state, this.token.token) | ||
| .catch(console.error) // eslint-disable-line no-console |
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.
This will need to be more robust. The main cases here are:
- No internet (status code 0?); probably show a warning/error
- Expired token (status code 401); open a popup, do the auth flow
- Spotify API is out (status code >= 400) ; maybe same flow as 1
- We introduced a bug (exception); maybe same flow as 1
We could put that logic in a service module, since there may be a number of places where we need to handle API errors.
| "@vue/cli-plugin-unit-jest": "^3.10.0", | ||
| "@vue/cli-service": "^3.8.0", | ||
| "@vue/test-utils": "^1.0.0-beta.29", | ||
| "babel-core": "7.0.0-bridge.0", |
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.
The vue cli added this dep for me, but I am pretty sure recent jest versions don't need this any more.
| /dist | ||
|
|
||
| # local env files | ||
| .env |
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 all the contents of this .gitignore are what were generated when Vue CLI created the app. Strange it didn't include .env
Summary