Skip to content

Advancements with postgres #2510

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

Merged
merged 46 commits into from
Aug 15, 2016
Merged

Advancements with postgres #2510

merged 46 commits into from
Aug 15, 2016

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented Aug 12, 2016

Initial status:

1011 specs, 0 failures, 423 pending specs

Aug. 15

987 specs, 0 failures, 161 pending specs

@codecov-io
Copy link

codecov-io commented Aug 12, 2016

Current coverage is 91.92% (diff: 85.18%)

Merging #2510 into master will increase coverage by 0.05%

@@             master      #2510   diff @@
==========================================
  Files            96         96          
  Lines         10867      10887    +20   
  Methods        1342       1345     +3   
  Messages          0          0          
  Branches       1756       1757     +1   
==========================================
+ Hits           9984      10008    +24   
+ Misses          883        879     -4   
  Partials          0          0          

Powered by Codecov. Last update 2f1ee21...fe52a53

@flovilmart flovilmart force-pushed the pg-advance branch 2 times, most recently from de912ed to 8e988ef Compare August 13, 2016 05:03
@@ -7,7 +7,7 @@ var FilesController = require('../src/Controllers/FilesController').default;


// Small additional tests to improve overall coverage
describe("GridStoreAdapter",() =>{
describe_only_db('mongo')("GridStoreAdapter",() =>{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only mongo as we don't start the DB when in PG mode

@flovilmart
Copy link
Contributor Author

@drew-gross feel free to review when you have a minute. I feel this adapter pretty messy... Want to knock down as many tests as possible and then maybe refactor / extract. What do you think?

expect(error.code).toEqual(Parse.Error.DUPLICATE_VALUE);
});
on_db('postgres', () => {
expect(error.code).toEqual('23505');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test should still be excluded in PG if it's not returning the right error code in PG. I thought on_db was going to be reserved for unsupported operations (like PG has no equivalent of mongo's $pullAll)

@drew-gross
Copy link
Contributor

Looks amazing to me. I have a few comments about test strategy that I think should be fixed. The code has become pretty complex, but I think the basic structure is mostly sound, so no need to fix any of that right away.

@flovilmart flovilmart changed the title WIP: Advancements with postgres Advancements with postgres Aug 15, 2016
@@ -13,6 +13,31 @@ const deepcopy = require('deepcopy');

const userSchema = SchemaController.convertSchemaToAdapterSchema({ className: '_User', fields: Object.assign({}, SchemaController.defaultColumns._Default, SchemaController.defaultColumns._User) });

describe_only_db('mongo')('miscellaneous', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved there as it tries to create a DB with mongo etc...

@flovilmart flovilmart merged commit c024928 into master Aug 15, 2016
@flovilmart flovilmart deleted the pg-advance branch August 15, 2016 20:48
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