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

Create tests for savedobject.js in prep of refactoring and changes #9127

Merged
merged 3 commits into from
Nov 28, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/test_utils/stub_mapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import MapperService from 'ui/index_patterns/_mapper';
import stubbedLogstashFields from 'fixtures/logstash_fields';
import sinon from 'auto-release-sinon';

export function stubMapper(Private, mockLogstashFields = Private(stubbedLogstashFields)) {
let stubbedMapper = Private(MapperService);

sinon.stub(stubbedMapper, 'getFieldsForIndexPattern', function () {
return Promise.resolve(mockLogstashFields.filter((field) => { return field.scripted === false; }));
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be slightly simplified:

return Promise.resolve(mockLogstashFields.filter(field => field.scripted === false));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! & done.

});

sinon.stub(stubbedMapper, 'clearCache', function () {
return Promise.resolve();
});

return stubbedMapper;
}
213 changes: 213 additions & 0 deletions src/ui/public/courier/__tests__/saved_object.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
/**
* Tests functionality in ui/public/courier/saved_object/saved_object.js
*/

import ngMock from 'ng_mock';
import expect from 'expect.js';
import sinon from 'auto-release-sinon';

import BluebirdPromise from 'bluebird';
import SavedObjectFactory from '../saved_object/saved_object';
import { stubMapper } from 'test_utils/stub_mapper';
import IndexPatternFactory from 'ui/index_patterns/_index_pattern';

describe('Saved Object', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This relates to #9161, I've seen a number of tests that would've used 'ui/courier/saved_object' for the describe, but I've also seen a number of tests do it this way. I'm not sure which way is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if there is a rule for this, but I think I prefer not specifying the path, otherwise you'd have to update the test if you move the file and I could see that easily being forgotten.

require('test_utils/no_digest_promises').activateForSuite();

let savedObjectFactory;
let IndexPattern;
let esStub;

beforeEach(ngMock.module('kibana'));
beforeEach(ngMock.inject(function ($injector, Private) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're injecting $injector and then using $injector.get('es') below as opposed to just injecting es?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fetch.js does something similar to what I did, getting es from the injector. Happy to write it another way that makes more sense but I'm not sure how to 'just inject es'. If you can give me a little code snippet to explain your suggestion I can add it.

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 you could change this to the following:

beforeEach(ngMock.inject(function (es, Private) {
  SavedObject = Private(SavedObjectFactory);
  IndexPattern = Private(IndexPatternFactory);
  esStub = es;

  mockEsService();
  stubMapper(Private);
}));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, awesome, done!

savedObjectFactory = Private(SavedObjectFactory);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I've more commonly seen this being imported as SavedObject and then used as a constructor in the createSavedObject below.

So instead of having:

let savedObject = {};
savedObjectFactory.call(savedObject, config);
return savedObject;

it'd become:

return new SavedObject(config);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, much better. I was confused with the factory and private stuff, making this overly complicated. Done!

IndexPattern = Private(IndexPatternFactory);
esStub = $injector.get('es');

mockEsService();
stubMapper(Private);
}));

/**
* Some default es stubbing to avoid timeouts and allow a default type of 'dashboard'.
*/
function mockEsService() {
// Allows the type 'dashboard' to be used.
// Unfortunately we need to use bluebird here instead of native promises because there is
// a call to finally.
sinon.stub(esStub.indices, 'getFieldMapping').returns(BluebirdPromise.resolve({
'.kibana' : {
'mappings': {
'dashboard': {}
}
}
}));

// Necessary to avoid a timeout condition.
sinon.stub(esStub.indices, 'putMapping').returns(BluebirdPromise.resolve());
};

/**
* Stubs some of the es retrieval calls so it returns the given response.
* @param {Object} mockDocResponse
*/
function stubESResponse(mockDocResponse) {
sinon.stub(esStub, 'mget').returns(BluebirdPromise.resolve({ docs: [mockDocResponse] }));
sinon.stub(esStub, 'index').returns(BluebirdPromise.resolve(mockDocResponse));
}

/**
* Creates a new saved object with the given configuration. Does not call init.
* @param {Object} config
* @returns {SavedObject} an instance of SavedObject
*/
function createSavedObject(config = {}) {
let savedObject = {};
savedObjectFactory.call(savedObject, config);
return savedObject;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: can we move these functions above the beforeEach? That way all of the "helper" vars and functions are defined before the mocha-specific beforeEach and define calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, done!


describe ('config', function () {

it('afterESResp is called', function () {
let afterESRespCallback = sinon.spy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout the tests, there's an inconsistent usage of const and let. There appears to be a pattern that generally let is used when an object will be 'mutated'; however, that's not the difference between const/let in javascript. let allows a reference to be reassigned per https://github.com/airbnb/javascript#references--prefer-const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Learn something new every day. Changed to const!

const config = {
type: 'dashboard',
afterESResp: afterESRespCallback
};

let savedObject = createSavedObject(config);
return savedObject.init().then(() => {
expect(afterESRespCallback.called).to.be(true);
});
});

it('init is called', function () {
let initCallback = sinon.spy();
const config = {
type: 'dashboard',
init: initCallback
};

let savedObject = createSavedObject(config);
return savedObject.init().then(() => {
expect(initCallback.called).to.be(true);
});
});

it('searchSource creates index when true', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create an assertion for when it's false?

describe('searchSource', () => {
  describe('when true, blah blah', () => {
  });

  describe('when false, blah blah', () => {
  });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const indexPatternId = 'testIndexPattern';
let afterESRespCallback = sinon.spy();

const config = {
type: 'dashboard',
afterESResp: afterESRespCallback,
searchSource: true,
indexPattern: indexPatternId
};

const mockDocResponse = {
_source: {},
_index: indexPatternId,
_type: 'test-type',
_id: indexPatternId,
found: true
};

stubESResponse(mockDocResponse);

let savedObject = createSavedObject(config);
expect(!!savedObject.searchSource.get('index')).to.be(false);

return savedObject.init().then(() => {
expect(afterESRespCallback.called).to.be(true);
const index = savedObject.searchSource.get('index');
expect(index instanceof IndexPattern).to.be(true);
expect(index.id).to.equal(indexPatternId);
});
});

describe('type', function () {
it('that is not specified throws an error', function () {
let config = {};

let savedObject = createSavedObject(config);
try {
savedObject.init();
expect(false).to.be(true);
} catch (err) {
expect(err).to.not.be(null);
}
});

it('that is invalid invalid throws an error', function () {
let config = {type: 'notypeexists'};

let savedObject = createSavedObject(config);
try {
savedObject.init();
expect(false).to.be(true);
} catch (err) {
expect(err).to.not.be(null);
}
});

it('that is valid passes', function () {
let config = {type: 'dashboard'};
return createSavedObject(config).init();
});
});

describe('defaults', function () {
it('applied to object with no id', function () {
let config = {
defaults: {testDefault: 'hi'},
type: 'dashboard'
};

let savedObject = createSavedObject(config);
return savedObject.init().then(() => {
expect(savedObject.searchSource).to.be(undefined);
expect(savedObject.defaults.testDefault).to.be(config.defaults.testDefault);
});
});

it('applied to source if an id is given', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a few more assertions:

  1. If id is provided, it's available as an id property on the savedObject instance.
  2. If it's 0, it's interpreted as undefined.
  3. If it's false, it's interpreted as undefined.
  4. If it's null, it's interpreted as undefined.
  5. If it's not provided, it defaults to undefined.

I'm just thinking we should document this (kinda odd) behavior in case anything depends on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const myId = 'myid';
const customDefault = 'hi';
const initialOverwriteMeValue = 'this should get overwritten by the server response';

let config = {
defaults: {
overwriteMe: initialOverwriteMeValue,
customDefault: customDefault
},
type: 'dashboard',
id: myId
};

const serverValue = 'this should override the initial default value given';

const mockDocResponse = {
_source: {
overwriteMe: serverValue
},
_index: myId,
_type: 'dashboard',
_id: myId,
found: true
};

stubESResponse(mockDocResponse);

let savedObject = createSavedObject(config);
return savedObject.init().then(() => {
expect(!!savedObject._source).to.be(true);
expect(savedObject.defaults.overwriteMe).to.be(initialOverwriteMeValue);
expect(savedObject._source.overwriteMe).to.be(serverValue);
expect(savedObject._source.customDefault).to.be(customDefault);
});
});
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Might also be good to add tests for other instance methods applyESResp, serialize, save, saveSource, destroy, and delete. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one for applyESResp. Save I'd prefer to get in the save/rename flow since I know what to test there. As for the others, it feels like I'd just be testing implementation details as opposed to expected results, so at least with my current understanding of this code, it doesn't seem super beneficial. (e.g. for delete, I could test that es.delete is called, but not that the item is actually deleted).

12 changes: 2 additions & 10 deletions src/ui/public/index_patterns/__tests__/_index_pattern.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import IndexedArray from 'ui/indexed_array';
import FixturesLogstashFieldsProvider from 'fixtures/logstash_fields';
import FixturesStubbedDocSourceResponseProvider from 'fixtures/stubbed_doc_source_response';
import DocSourceProvider from 'ui/courier/data_source/doc_source';
import IndexPatternsMapperProvider from 'ui/index_patterns/_mapper';
import UtilsMappingSetupProvider from 'ui/utils/mapping_setup';
import IndexPatternsIntervalsProvider from 'ui/index_patterns/_intervals';
import IndexPatternsIndexPatternProvider from 'ui/index_patterns/_index_pattern';
import NoDigestPromises from 'test_utils/no_digest_promises';
import { stubMapper } from 'test_utils/stub_mapper';

describe('index pattern', function () {
NoDigestPromises.activateForSuite();
Expand Down Expand Up @@ -40,15 +40,7 @@ describe('index pattern', function () {
DocSource = Private(DocSourceProvider);
sinon.stub(DocSource.prototype, 'doIndex');
sinon.stub(DocSource.prototype, 'fetch');

// stub mapper
mapper = Private(IndexPatternsMapperProvider);
sinon.stub(mapper, 'getFieldsForIndexPattern', function () {
return Promise.resolve(_.filter(mockLogstashFields, { scripted: false }));
});
sinon.stub(mapper, 'clearCache', function () {
return Promise.resolve();
});
mapper = stubMapper(Private, mockLogstashFields);

// stub mappingSetup
mappingSetup = Private(UtilsMappingSetupProvider);
Expand Down
6 changes: 6 additions & 0 deletions src/ui/public/promises/__tests__/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ describe('Promise service', function () {
});
});

it('Promise.resolve', function (done) {
Promise.resolve(true).then(() => { done(); });
// Ugly, but necessary for promises to resolve: https://github.com/angular/angular.js/issues/12555
$rootScope.$apply();
});

describe('Promise.fromNode', function () {
it('creates a callback that controls a promise', function () {
let callback;
Expand Down