[Security] tests for API keys app by alisonelizabeth · Pull Request #48560 · elastic/kibana

+import { initApiKeysApi } from '../api_keys';
+import * as ClientShield from '../../../../../../../server/lib/get_client_shield';
+
+describe('API Keys routes', () => {


We've generally tried to write all unit tests in jest as opposed to mocha and no longer rely on sinon. Can we change these to use jest without sinon?

kobelb · 2019-11-01T15:00:09Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+
+  afterEach(() => sandbox.restore());
+
+  describe('privileges', () => {


nit: These tests are missing the assertions to ensure that callWithRequest is invoked with the proper parameters... it's possible to change

callWithRequest( request, 'shield.hasPrivileges', { body: { cluster: [ 'manage_security', 'manage_api_key', ], }, } )

to

callWithRequest( request, 'shield.hasPrivileges', { body: { foo: true, }, } )

and still have these tests pass.

kobelb · 2019-11-01T16:19:54Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+
+      const { areApiKeysEnabled } = await privilegesRoute.handler(request);
+
+      expect(areApiKeysEnabled).to.be(true);


Suggested change

expect(areApiKeysEnabled).to.be(true);

expect(areApiKeysEnabled).to.be(false);

kobelb · 2019-11-01T16:20:01Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+          sinon.match.same(request),
+          'shield.getAPIKeys',
+        )
+        .resolves(apiKeysDisabledError);


Suggested change

.resolves(apiKeysDisabledError);

.rejects(apiKeysDisabledError);

kobelb · 2019-11-01T16:24:06Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+      expect(areApiKeysEnabled).to.be(true);
+    });
+
+    it('sets isAdmin to true if a user has the manage_security or manage_api_key privilege', async () => {


nit: This isn't really testing whether or not the user has the manage_security cluster privilege or the manage_api_key privilege, it's testing what happens when they have both. We're already testing this with "returns areApiKeysEnabled and isAdmin"

kobelb · 2019-11-01T16:30:48Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+      expect(errors).to.have.length(0);
+    });
+
+    it('returns a list of errors encountered when trying to invalidate each key', async () => {


Can we change this test to only throw an error when invalidating the first api key, and ensure the second api key is still invalidated?

kobelb · 2019-11-01T16:35:41Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+      });
+    });
+
+    it('only returns valid keys', async () => {


nit: This test is also missing the assertions to ensure that callWithRequest is invoked with the proper parameters...

kobelb · 2019-11-01T16:38:00Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+        .resolves(MOCK_API_KEYS_WITH_INVALIDATED_RESPONSE);
+
+      const { apiKeys } = await getRoute.handler(request);
+      expect(apiKeys).to.have.length(1);


Suggested change

expect(apiKeys).to.have.length(1);

expect(apiKeys).to.have.length(1);

expect(apiKeys[0]).to.equal(MOCK_API_KEYS_WITH_INVALIDATED_RESPONSE.api_keys[0]);

kobelb · 2019-11-01T16:44:06Z

x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/api_keys.js

+
+      const { itemsInvalidated, errors } = await invalidateRoute.handler(request);
+
+      expect(itemsInvalidated).to.have.length(MOCK_INVALIDATE_API_KEYS.length);


Suggested change

expect(itemsInvalidated).to.have.length(MOCK_INVALIDATE_API_KEYS.length);

expect(itemsInvalidated).to.have.length(MOCK_INVALIDATE_API_KEYS.length);

expect(itemsInvalidated).to.equal(MOCK_INVALIDATE_API_KEYS);

kobelb · 2019-11-01T17:25:20Z

...lugins/security/public/views/management/api_keys_grid/components/api_keys_grid_page.test.tsx

+  it('renders a loading state when fetching API keys', async () => {
+    const wrapper = mountWithIntl(<ApiKeysGridPage />);
+
+    expect(wrapper.find(SectionLoading)).toHaveLength(1);


Would using a data-test-subj here let us get away from having to use // eslint-disable-next-line @kbn/eslint/no-restricted-paths above?

kobelb · 2019-11-01T17:30:38Z

Thanks so much for adding these @alisonelizabeth!

alisonelizabeth · 2019-11-18T17:32:20Z

@elasticmachine merge upstream

alisonelizabeth · 2019-11-18T17:36:16Z

@kobelb thanks for the review! I finally got around to addressing your feedback. I converted the API tests to jest and removed sinon. Do you think you could take another look?

elasticmachine · 2019-11-18T18:50:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0a9e3d5

alisonelizabeth · 2019-11-20T18:07:48Z

@elasticmachine merge upstream

elasticmachine · 2019-11-20T19:29:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9192155

alisonelizabeth added chore Feature:Users/Roles/API Keys v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes labels Oct 17, 2019

alisonelizabeth force-pushed the api_keys_tests branch from 37d684e to 620c839 Compare October 17, 2019 20:34

add API keys tests

8d4347e

alisonelizabeth force-pushed the api_keys_tests branch from 620c839 to 8d4347e Compare October 21, 2019 19:01

alisonelizabeth added the v7.6.0 label Oct 21, 2019

alisonelizabeth marked this pull request as ready for review October 21, 2019 19:02

alisonelizabeth requested a review from a team as a code owner October 21, 2019 19:02

Merge branch 'master' into api_keys_tests

abd167b

alisonelizabeth requested review from cuff-links and kobelb October 21, 2019 20:47

Merge branch 'master' into api_keys_tests

93feb2c

Merge branch 'master' into api_keys_tests

059d951

cuff-links self-assigned this Oct 30, 2019

cuff-links approved these changes Oct 30, 2019

View reviewed changes

Merge branch 'master' into api_keys_tests

a2513ef

kobelb reviewed Nov 1, 2019

View reviewed changes

address review feedback

367795d

Merge branch 'master' into api_keys_tests

0a9e3d5

kobelb approved these changes Nov 20, 2019

View reviewed changes

Merge branch 'master' into api_keys_tests

9192155

alisonelizabeth merged commit 3561029 into elastic:master Nov 20, 2019

alisonelizabeth deleted the api_keys_tests branch November 20, 2019 19:32

alisonelizabeth mentioned this pull request Nov 20, 2019

[7.x] [Security] tests for API keys app (#48560) #51217

Merged

alisonelizabeth added a commit that referenced this pull request Nov 20, 2019

[Security] tests for API keys app (#48560) (#51217)

b9e2fbf


		afterEach(() => sandbox.restore());

		describe('privileges', () => {


		const { areApiKeysEnabled } = await privilegesRoute.handler(request);

		expect(areApiKeysEnabled).to.be(true);

	expect(areApiKeysEnabled).to.be(true);
	expect(areApiKeysEnabled).to.be(false);

	.resolves(apiKeysDisabledError);
	.rejects(apiKeysDisabledError);

	expect(apiKeys).to.have.length(1);
	expect(apiKeys).to.have.length(1);
	expect(apiKeys[0]).to.equal(MOCK_API_KEYS_WITH_INVALIDATED_RESPONSE.api_keys[0]);


		const { itemsInvalidated, errors } = await invalidateRoute.handler(request);

		expect(itemsInvalidated).to.have.length(MOCK_INVALIDATE_API_KEYS.length);

	expect(itemsInvalidated).to.have.length(MOCK_INVALIDATE_API_KEYS.length);
	expect(itemsInvalidated).to.have.length(MOCK_INVALIDATE_API_KEYS.length);
	expect(itemsInvalidated).to.equal(MOCK_INVALIDATE_API_KEYS);

Conversation

alisonelizabeth commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 17, 2019

Uh oh!

elasticmachine commented Oct 17, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Oct 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 21, 2019

💚 Build Succeeded

Uh oh!

alisonelizabeth commented Oct 21, 2019

Uh oh!

elasticmachine commented Oct 21, 2019

💚 Build Succeeded

Uh oh!

alisonelizabeth commented Oct 23, 2019

Uh oh!

elasticmachine commented Oct 23, 2019

💚 Build Succeeded

Uh oh!

alisonelizabeth commented Oct 28, 2019

Uh oh!

elasticmachine commented Oct 28, 2019

💚 Build Succeeded

Uh oh!

cuff-links commented Oct 30, 2019

Uh oh!

cuff-links left a comment

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Oct 31, 2019

Uh oh!

elasticmachine commented Oct 31, 2019

💚 Build Succeeded

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 1, 2019

Uh oh!

alisonelizabeth commented Nov 18, 2019

Uh oh!

alisonelizabeth commented Nov 18, 2019

Uh oh!

elasticmachine commented Nov 18, 2019

💚 Build Succeeded

Uh oh!

alisonelizabeth commented Nov 20, 2019

Uh oh!

elasticmachine commented Nov 20, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

alisonelizabeth commented Oct 17, 2019 •

edited

Loading