Create tests for savedobject.js in prep of refactoring and changes by stacey-gammon · Pull Request #9127 · elastic/kibana

stacey-gammon · 2016-11-17T17:42:57Z

No description provided.

cjcenizal

Looks great! I have some suggestions but I'll leave it up to you to decide to implement or not.

cjcenizal · 2016-11-17T20:15:16Z

src/test_utils/stub_mapper.js

+  let stubbedMapper = Private(MapperService);
+
+  sinon.stub(stubbedMapper, 'getFieldsForIndexPattern', function () {
+    return Promise.resolve(mockLogstashFields.filter((field) => { return field.scripted === false; }));


This can be slightly simplified:

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

Nice! & done.

cjcenizal · 2016-11-21T20:15:35Z

src/ui/public/courier/__tests__/saved_object.js

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


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.

indeed, done!

cjcenizal · 2016-11-21T20:29:38Z

src/ui/public/courier/__tests__/saved_object.js

+        });
+      });
+
+      it('applied to source if an id is given', function () {


Can we add a few more assertions:

If id is provided, it's available as an id property on the savedObject instance.

If it's 0, it's interpreted as undefined.

If it's false, it's interpreted as undefined.

If it's null, it's interpreted as undefined.

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.

cjcenizal · 2016-11-21T20:32:41Z

src/ui/public/courier/__tests__/saved_object.js

+      });
+    });
+
+    it('searchSource creates index when true', function () {


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

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

cjcenizal · 2016-11-21T21:31:08Z

src/ui/public/courier/__tests__/saved_object.js

+      });
+    });
+  });
+});


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

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).

kobelb · 2016-11-21T22:01:31Z

src/ui/public/courier/__tests__/saved_object.js

+import { stubMapper } from 'test_utils/stub_mapper';
+import IndexPatternFactory from 'ui/index_patterns/_index_pattern';
+
+describe('Saved Object', function () {


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.

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.

kobelb · 2016-11-22T14:13:24Z

src/ui/public/courier/__tests__/saved_object.js

+  let esStub;
+
+  beforeEach(ngMock.module('kibana'));
+  beforeEach(ngMock.inject(function ($injector, Private) {


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

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.

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); }));

ah, awesome, done!

kobelb · 2016-11-22T14:48:01Z

src/ui/public/courier/__tests__/saved_object.js

+
+  beforeEach(ngMock.module('kibana'));
+  beforeEach(ngMock.inject(function ($injector, Private) {
+    savedObjectFactory = Private(SavedObjectFactory);


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);

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

kobelb · 2016-11-22T14:50:43Z

src/ui/public/courier/__tests__/saved_object.js

+  describe ('config', function () {
+
+    it('afterESResp is called', function () {
+      let afterESRespCallback = sinon.spy();


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

Learn something new every day. Changed to const!

kobelb · 2016-11-22T19:44:59Z

LGTM!!!

Backports PR #9127 **Commit 1:** Create tests for savedobject.js in prep of refactoring and changes * Original sha: a6fd8f0 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-11-17T17:06:59Z **Commit 2:** Address code review comments * Original sha: fce433d * Authored by Stacey Gammon <gammon@elastic.co> on 2016-11-22T19:19:20Z **Commit 3:** Remove needless injector param, replace with es * Original sha: 834cb27 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-11-22T19:43:06Z

…9232) Backports PR #9127 **Commit 1:** Create tests for savedobject.js in prep of refactoring and changes * Original sha: a6fd8f0 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-11-17T17:06:59Z **Commit 2:** Address code review comments * Original sha: fce433d * Authored by Stacey Gammon <gammon@elastic.co> on 2016-11-22T19:19:20Z **Commit 3:** Remove needless injector param, replace with es * Original sha: 834cb27 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-11-22T19:43:06Z

stacey-gammon added the review label Nov 17, 2016

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

a6fd8f0

stacey-gammon force-pushed the saved_object_tests branch from 175fc03 to a6fd8f0 Compare November 17, 2016 18:19

stacey-gammon assigned kobelb and cjcenizal Nov 17, 2016

stacey-gammon mentioned this pull request Nov 17, 2016

Save/rename flow fix #9087

Merged

tbragin added the :Sharing label Nov 19, 2016

cjcenizal approved these changes Nov 21, 2016

View reviewed changes

kobelb reviewed Nov 22, 2016

View reviewed changes

Stacey Gammon added 2 commits November 22, 2016 14:19

Address code review comments

fce433d

Remove needless injector param, replace with es

834cb27

stacey-gammon merged commit 469a211 into elastic:master Nov 28, 2016

elastic-jasper mentioned this pull request Nov 28, 2016

[5.x] Create tests for savedobject.js in prep of refactoring and changes #9232

Merged

stacey-gammon added the v5.2.0 label Nov 30, 2016

stacey-gammon deleted the saved_object_tests branch December 19, 2016 15:45

Comments

Conversation

stacey-gammon commented Nov 17, 2016

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb commented Nov 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants