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 and fixes #162

Merged
merged 5 commits into from
Sep 6, 2017
Merged

Conversation

ryanthegiantlion
Copy link
Contributor

Fixes for #125, #137 and #161

Also added tests for all the above.

…ections do not do clean up. In addition to that some of the individual tests only do clean up when it succeeds (In the .then callback). The result is that when a test fails it causes many other tests to fail and it can be difficult to see what is breaking. This change should alleviate some of that.
@daffl
Copy link
Member

daffl commented Sep 6, 2017

This looks great, thank you! I'll let @DesignByOnyx have a look and then we can get it out.

@DesignByOnyx
Copy link
Contributor

This looks awesome. Thanks for taking the time to do this. I see no reason not to merge.

For the sake of discussion, by introducing dialect-specific tests we have legitimized dialect-specific code. I was previously trying to avoid this, but I now see a lot of value in having optimizations for different engines. We should eventually start running the tests against all dialects so that we can catch any discrepancies quickly. I will create a separate issue to track that. Thanks for getting it started for us @ryanthegiantlion!

@daffl
Copy link
Member

daffl commented Sep 6, 2017

@DesignByOnyx, @ryanthegiantlion already got you there. CI tests for SQLite, MySQL and Postgres have been added in #160

@daffl daffl merged commit d98b10e into feathersjs-ecosystem:master Sep 6, 2017
@daffl
Copy link
Member

daffl commented Sep 6, 2017

Released as v2.3.1

@DesignByOnyx
Copy link
Contributor

Ah, I missed that PR for the dialect-specific tests. My bad.

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