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

Detect deleteAll support in KVAO tests #1218

Merged
merged 1 commit into from
Jan 12, 2017

Conversation

superkhau
Copy link
Contributor

Clear CacheItem data between tests if connector supports deleteAll. Tests fail with 501 if this check is not used.

@superkhau superkhau requested a review from bajtos January 10, 2017 08:21
@superkhau superkhau force-pushed the clear-cache-if-delete-all-supported branch from bde2a21 to a09667c Compare January 10, 2017 08:21
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Nice 👍

@@ -11,6 +11,8 @@ module.exports = function(dataSourceFactory, connectorCapabilities) {
let CacheItem;
beforeEach(function unpackContext() {
CacheItem = helpers.givenCacheItem(dataSourceFactory);
if (helpers.connectorSupports('deleteAll', dataSourceFactory))
return CacheItem.deleteAll();
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think we should move CacheItem.deleteAll into givenCacheItem.

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

@superkhau superkhau self-assigned this Jan 10, 2017
@superkhau superkhau force-pushed the clear-cache-if-delete-all-supported branch 6 times, most recently from 615f7b1 to 0bab394 Compare January 10, 2017 09:10
@superkhau superkhau closed this Jan 10, 2017
@superkhau superkhau removed the review label Jan 10, 2017
@superkhau superkhau reopened this Jan 10, 2017
@superkhau superkhau added this to the Sprint 26 milestone Jan 10, 2017
@superkhau superkhau force-pushed the clear-cache-if-delete-all-supported branch from 0bab394 to ff34d25 Compare January 10, 2017 09:20
return CacheItem;
};

exports.givenAnotherModel = function(dataSourceFactory) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's time to create a more generic givenModel method.

key: String,
value: 'any',
});
if ('deleteAll' in dataSource.connector)
CacheItem.deleteAll();
Copy link
Member

Choose a reason for hiding this comment

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

You need to wait until deleteAll returns before starting the tests.

I think we need to change givenCacheItem to return a promise now.

exports.givenCacheItem = function(dataSourceFactory) {
  // note - most of this code should be moved to `givenModel`
  // ...
  const p = 'deleteAll' in dataSource.connector ? 
    CacheItem.deleteAll() : Promise.resolve;

    return p.then(() => CacheItem);
};

@@ -11,7 +11,6 @@ module.exports = function(dataSourceFactory, connectorCapabilities) {
let CacheItem;
beforeEach(function unpackContext() {
CacheItem = helpers.givenCacheItem(dataSourceFactory);
return CacheItem.deleteAll();
Copy link
Member

Choose a reason for hiding this comment

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

beforeEach(function unpackContext() {
  return helpers.givenCacheItem(dataSourceFactory)
    .then(ModelCtor => CacheItem = ModelCtor);
});

@@ -23,12 +23,13 @@ module.exports = function(dataSourceFactory, connectorCapabilities) {
});

it('does not remove data from other existing models', function() {
var AnotherModel = dataSourceFactory().createModel('AnotherModel');
var AnotherModel = helpers.givenAnotherModel(dataSourceFactory);
Copy link
Member

Choose a reason for hiding this comment

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

helpers.givenModel(dataSourceFactory, 'AnotherModel')
  .then(ModelCtor => {
    AnotherModel = ModelCtor;
    return helpers.givenKeys(CacheItem, ['key1', 'key2']);
  })
  .then(() => helpers.givenKeys(AnotherModel, ['key3', 'key4']))
  // ...

Write

@superkhau superkhau force-pushed the clear-cache-if-delete-all-supported branch 2 times, most recently from ebb786f to e29c9c7 Compare January 10, 2017 10:03
beforeEach(function unpackContext() {
CacheItem = helpers.givenCacheItem(dataSourceFactory);
beforeEach(function setupCacheItem() {
helpers.givenCacheItem(dataSourceFactory)
Copy link
Member

Choose a reason for hiding this comment

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

missing return

@superkhau superkhau force-pushed the clear-cache-if-delete-all-supported branch from e29c9c7 to 0149c42 Compare January 10, 2017 11:16
@superkhau
Copy link
Contributor Author

@bajtos PTAL, I assume this is good now? CI fails are unrelated.

@superkhau
Copy link
Contributor Author

@slnode test please

Clear CacheItem data between tests if connector supports `deleteAll`.
Tests fail with 501 if this check is not used.
@superkhau superkhau force-pushed the clear-cache-if-delete-all-supported branch from 0149c42 to 0b93c5c Compare January 12, 2017 00:30
@superkhau
Copy link
Contributor Author

superkhau commented Jan 12, 2017

@bajtos Gonna land this as everything is passing and I addressed all your concerns in the previous comments so I can test the KVXS PR.

@superkhau
Copy link
Contributor Author

superkhau commented Jan 12, 2017

Follow up issue for oracle fail at loopbackio/loopback-connector-oracle#100

@superkhau superkhau merged commit 736a4ad into master Jan 12, 2017
@superkhau superkhau deleted the clear-cache-if-delete-all-supported branch January 12, 2017 03:31
@superkhau superkhau removed the review label Jan 12, 2017
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants