search

balderdashy / waterline-adapter-tests

API integration tests for Waterline adapters
MIT License
16 stars 41 forks source link

Added a Semantic test for updates when unique attributes are present. #63

Closed marnusw closed 9 years ago

marnusw commented 9 years ago

When updating multiple records based on a given find criteria there won't be an id specified in the object with the new values. This could make it tricky, when a model has attributes marked as unique, to establish whether an existing record with a duplicate value is actually the record being updated, which is not actually a violation.

The sails-memory adapter has such a bug, and this test was added to flag it. I will reference this PR from the one I submit with the fix for the bug on the sails-memory repo.

marnusw commented 9 years ago

I see the Travis build fails now because this issue is present in a number of the adapters. If unique attributes aren't supported for all adapters, is there a way to detect whether it is for a given adapter and run this test only in that case?

dmarcelino commented 9 years ago

Hi @marnusw, thanks for your PR. Indeed unique is not supported by all adapters, as described in the docs:

This is a database level constraint so in most cases a unique index will be created in the underlying data-store.

As to regards to detection I don't think there is currently a way to do it. In the future, a possible solution for this is by placing this test in a /features/unique folder and then add "features": ["unique"] to the adapter's package.json. Such functionality is not decided yet and is being discussed in PR #64. I suggest you subscribe to it and then adapt this solution depending on the outcome.

dmarcelino commented 9 years ago

@marnusw, PR #64 has been merged, do you mind refactoring this as a "feature"? Thanks!

marnusw commented 9 years ago

Will do.

marnusw commented 9 years ago

I've refactored this to a feature test for the unique attribute option. Perhaps simply calling the feature "unique" is too vague? Let me know if it should change.

At present these tests pass for sails-disk, but will fail for sails-memory until balderdashy/sails-memory#19 is merged. Once this PR is merged I will update it to include unique as a feature.

dmarcelino commented 9 years ago

I've refactored this to a feature test for the unique attribute option. Perhaps simply calling the feature "unique" is too vague? Let me know if it should change.

Well... the flag is called unique in waterline and the related constraint has the same name in SQL so people will "hopefully" associate it with the right thing, given the context :)

The tests looks good :white_check_mark:

Have you tested this with any adapter besides sails-memory/disk?

marnusw commented 9 years ago

I believe this is now ready to merge once #68, providing the '.' feature notation and knowledge of "core" folders, is merged.

dmarcelino commented 9 years ago

@devinivy, can you take a second look please?

marnusw commented 9 years ago

@dmarcelino Now that the "." notation is merged via #68 I'll run this test for all adapters as well and report back over/after the weekend! With that and @devinivy's go-ahead it should be ready too.

dmarcelino commented 9 years ago

Nice, thanks! :beers:

marnusw commented 9 years ago

So I ended up doing it now... :blush:

The feature wasn't enabled for Redis, but it was for all other adapters. It all seems good, and sails-memory will be fixed with the PR I have open that side. 

 -----------------------------------
| core modules            | version |
|-------------------------|---------|
| waterline               | 0.10.24 |
| - anchor                |  0.10.3 |
| - waterline-schema      |  0.1.17 |
| waterline-adapter-tests | 5656f2c |
 -----------------------------------

 ------------------------------------------------------------------
| adapter          | version | result | failed | total | wl-sequel |
|------------------|---------|--------|--------|-------|-----------|
| sails-postgresql | 30317cd | passed |      0 |   344 |     0.4.0 |
| sails-memory     | c7b0610 | failed |      1 |   319 |           |
| sails-disk       | e95628b | passed |      0 |   324 |           |
| sails-mongo      | 021ec02 | passed |      0 |   321 |           |
| sails-mysql      | 01c1a0e | passed |      0 |   349 |     0.4.0 |
| sails-redis      | 72ced91 | passed |      0 |   317 |           |
 -------------------------------------------------------------------
devinivy commented 9 years ago

To be clear– in the test results that we're seeing, all those adapters were tagged with the unique feature aside from sails-redis, correct? And all the tests pass Travis CI because none of the adapters are tagged with the unique feature currently. Should we make issues on postgres, disk, mongo, and mysql? And ensure the tag makes it into your PR on memory?

This PR looks good!

marnusw commented 9 years ago

@devinivy that is correct.

And yes, I'll update my sails-memory PR with the tags, and I think adding issues for the others is a good idea. I'll do them all when I update the memory PR.

dmarcelino commented 9 years ago

Thanks again @marnusw and @devinivy!

@particlebanana, can you please release a new version of waterline-adapter-tests so we can test autoIncrement and unique features on the adapters?