Skip to content
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

Tests & Updates #71

Merged
merged 64 commits into from
Jan 21, 2018
Merged

Tests & Updates #71

merged 64 commits into from
Jan 21, 2018

Conversation

billdybas
Copy link
Member

@billdybas billdybas commented Jan 4, 2018

Adds integration tests for routes, testing the public API.

Through these tests, I uncovered 2 bugs: #68 and #67. These bugs will not be fixed in this PR.

Does not test headcounts, mentoring, shifts, specialties as they will be slated for removal per #70.


Once these tests are merged in and the bugs fixed, we should require tests to pass in our CI before PR's can be merged.

Also updates a number of packages used in the API, accounting for any breaking changes in doing so.

Requires a Sequelize migration, so our db's should be backed up before this PR is applied.
  
Closes #46 

*Before:* Model Associations were defined in a function
which was called in `app.js` to initialize them.

*Issue:* This was currently working, as during normal execution,
`models()` was only called that one time. However, in testing,
we need to reference `app` multiple times, so the Model Assocations
were being redefined in each test. This was causing tests to break,
as Sequelize was complaining that in `models/index.js` `addScope`
was adding duplicate scopes to Models (since it was being called
multiple times).

*Now:* We remove the function wrapping the Model Associations
so that the first file which imports them defines them once,
and subsequent times that file is loaded, Model Associations
will not be redefined.

Old Usage:
```js
import models from './models';
models();
```

New Usage:
```js
import './models';
```
Change 'expiresInMinutes' to 'expiresIn' b/c
of deprecation notice. Also uses '2 days' for
more readability than multiplying numbers.
After updating JWT packages, additional
work is necessary to create refresh tokens.
If 'findOrCreate' was creating a new User,
'image' was set to 'undefined', therefore
no 'image' property was sent in the response body.

Additionally, if 'req.body.user' was not
provided, it would override any image value set to 'undefined'.
- Expects Named Exports
- Needs Babel
We want references between quotes and tags to be
deleted if quotes or tags are deleted. It doesn't
make sense to have a quote reference a tag that
doesn't exist or a tag to reference a quote
that doesn't exist.

Also without this constraint, there were issues
in testing removing the rows from these tables.

Had to handle quirks between SQLite and Postgres
for the migration. See the commit for details.

May be able to rewrite the migration when we
upgrade to a newer version of Sequelize which
has support in the queryInterface for modifying constraints.
Bug in total counts
It was originally correct. I was unaware
at the time we use the Google API client
on the frontend and then send the user's
info to the backend; we don't actually
perform the authorization request on the backend.
To better mirror prod. Also fix typo.
Extension Sets Accept Header
@billdybas billdybas requested a review from vuzDylan January 4, 2018 09:14
@vuzDylan
Copy link
Contributor

vuzDylan commented Jan 4, 2018

Wow the node-api really is a giant monolith isn't it

Copy link
Contributor

@vuzDylan vuzDylan left a comment

Choose a reason for hiding this comment

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

Seems good

@vuzDylan
Copy link
Contributor

vuzDylan commented Jan 4, 2018

I like how bob ross made it in

Postgres doesn't like when ids are hardcoded, and
its auto incrementing gets thrown off, so we get
unique constraint errors when creating new events.

As a result, we're dropping testing support for SQLite
since the ids were hardcoded in the first place since
SQLite doesn't support restarting numbering.

This aligns the testing environment w/ our production
environment, but does require running Postgres locally.
I've added some resources in the README for getting
started w/ Postgres for those new to web development.

This issue only happens in testing, so we can still use
SQLite for running the API locally since one shouldn't
need to restart the primary key numbering.
@billdybas billdybas changed the title [WIP] Tests & Updates Tests & Updates Jan 21, 2018
@billdybas billdybas merged commit 4437d17 into devlop Jan 21, 2018
@billdybas billdybas deleted the updates-and-tests branch January 21, 2018 02:03
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.

2 participants