[Cloud Security] Adding MSW for mocking server responses in React integration tests

[opauloh]

Summary

It closes #183977

This PR introduces the MSW library into Kibana and setups for MSW usage with Jest for integration testing of React components in the Cloud Security Posture plugin.

It also adds the setup for the initial handlers, and configures a test for the <NoFindingsStates/> components using MSW to exemplify how the library works.

Problem Statement

Currently, integration tests for React components that interact with the server are hard to write and maintain, as they often require mocking functions implementation and responses, this can lead to tests that do not accurately verify the intended functionality and can be hard to maintain as the implementation of the functions changes.

This leads to situations our team faces now, where due to the difficult maintainability of integration tests, we rely much more on End-to-End tests, and maintaining those many End-to-End comes with its own set of tradeoffs, as oftentimes End-to-End tests are detected by the CI as failing or flaky, and as flakiness can happen in End-to-end tests due to its nature of multiple integrated systems, this concept proposes that it's better to reserve End-to-end tests for the features in the most critical path and tests that test multiple integrated systems as those will benefit most of the end-to-end testing. For all the other tests we should focus on unit and integration tests.

How MSW works

MSW is a library that allows you to mock server responses in your tests, it works by intercepting the requests made by the client and returning the mocked responses, this way we can test how the client behaves in different states of the lifecycle such as loading, error, and success.

This proposes that we should use MSW to enhance our integration tests, and give preference to writing integration tests over End-to-End tests whenever possible, but this doesn't mean that we should stop writing end-to-end tests, as end-to-end tests are still important for the features in the most critical path and tests that tests multiple integrated systems.

MSW Diagram

Here's a diagram that shows how MSW works with Jest tests:

%%{init:{'themeCSS':' g:nth-of-type(3) rect.actor { fill: #eee; };g:nth-of-type(7) rect.actor { fill: #eee; };'}}%%

sequenceDiagram
    participant ReactComponent as React Component
    participant API as API
    participant MSW as MSW Mock Server
    participant Jest as Jest Test

    Jest->>ReactComponent: Setup component test and mock providers
    Jest->>MSW: Setup Mock Server
    Note over Jest,MSW: start listening for requests
    activate MSW
    ReactComponent->>API: Make API Call
    Note over ReactComponent,API: loading state
    activate API
    MSW-->>API: Intercepts API Call
    deactivate API
    alt is success
        MSW-->>ReactComponent: Send Mocked success Response
    else is error
        MSW-->>ReactComponent: Send Mocked error Response
    end
    deactivate MSW
    ReactComponent-->>Jest: Receive Mocked data and render

Loading

Documentation

• Refer to this PR containing the documentation of how MSW works and how to use it.
• Refer to this presentation to understand the main motivations behind this proposal.

How to test it

yarn test:jest x-pack/plugins/cloud_security_posture/public/components/no_findings_states.test.tsx

Screenshot

Intercepted requests logged with {debug: true}:

[elasticmachine]

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

[maxcold]

have you considered collocating the mocks with the API implementation/definition as we own both? Is it possible? eg. have them as a part of x-pack/plugins/cloud_security_posture/server/routes/status and x-pack/plugins/cloud_security_posture/server/routes/benchmarks so it's more like consumer/provider relationship in contract testing. So the provider (backend) shares the artifacts, mocks in this case, for testing. In our case, I hope it will allow for a more structured approach. Eg. right now we have routes benchmarks and benchmark_rules but benchmark_handlers and it's hard to understand without looking at the code what it covers

[seanrathier]

@maxcold you bring up a good point about collocation. It was not something we thought about during the POC, however, @opauloh may have while continuing the work.

I agree with your point.

Adoption in the CSP plugin may become cumbersome to follow the associated handlers, collocating them can give a visual clue as to what the handlers are mocking.

import { statusNotInstalled } from '../../../../server/routes/status/status.mock.handlers';
...
server.use(statusNotInstalled);

```

[opauloh]

Thanks @maxcold and @seanrathier for the inputs on the naming conventions and colocation, I elaborated the improvement suggestions into two options for us to analyze and discuss the outcomes:

Option 1 - Centralize all Handlers

All handlers remain located within the public/test/mock_server/handlers folder, in favour of centralizing all mock-server-related logic.

Each handler must follow the convention of being named after the HTTP path they handle:

- public/test/mock_server/handlers
    - /internal
      - /cloud_security_posture
        	- status.handlers.mock.ts
        	- benchmarks.handlers.mock.ts
        - ...
      - /data_view
        - fields.handlers.mock.ts
      - /bsearch
      	- configurations.handlers.mock.ts
      	- vulnerabilities.handlers.mock.ts
    - /api
      - /licensing
        - info.handlers.mock.ts
      - /fleet
        - /epm
          - /packages
          	- cloud_security_posture.handlers.mock.ts
             - mocks/cloud_security_posture.mock.json
      - ...

The strict naming convention enhances the original proposal by following strict naming conventions for the path of the handlers. In this option, the handler files should be named after the path they handle. For example, the handler for the path /api/fleet/epm/packages/cloud_security_posture should be named cloud_security_posture.handlers.mock.js And be placed in the public/test/mock_server/handlers/api/fleet/epm/packages folder.

This proposal makes it easier to know that all code related to the mock server is in one central place. However in some cases (such as POST APIs like /internal/bsearch) we would need to go out of the strict convention.

And as @maxcold pointed out:

Having this structure which mimics the path, but not exactly, confused me until I looked deeper into the code

Option 2 - Colocate Handlers

All handlers derived from APIs we own (i.e: internal/cloud_security_posture/*) we colocate alongside the server route that's responsible for the API.

- server/routes
  - status
  	- status.ts
  	- status.test.ts
    - status.handlers.mock.ts
  - benchmarks
  	- benchmarks.ts
  	- benchmarks.test.ts
    - benchmarks.handlers.mock.ts
  - ...

While the handlers that are common and derive from APIs we do not own, we centralize them under the public/test/mock_server/handlers folder, following a flexible naming convention according to the domain.

- public/test/mock_server
  - /handlers
  	  - index.ts
      - data_view.handlers.mock.ts
      - licensing.handlers.mock.ts
      - fleet.handlers.mock.ts
      - /mocks
      	- cloud_security_posture.mock.json

And handlers that are related to APIs we do not own but are very specific to tests are stored within the tests

- public/pages
  - /configurations 
  	  - configurations.tsx
  	  - configurations.test.tsx
  	  - configurations.handlers.mock.ts (containing handlers for `/internal/bsearch`)
  - /vulnerabilities
  	  - vulnerabilities.tsx
  	  - vulnerabilities.test.tsx
  	  - vulnerabilities.handlers.mock.ts (containing handlers for `/internal/bsearch`)

While this option splits the handlers in multiple locations, it uses well the concept of co-location where we place code as close to where it's relevant as possible.

The handlers of APIs we own with the server route that's responsible for the API make it easier to understand the relationship between the server route and the mock handler. As well we prepare for further improvements such as validating the schema of the mocks against the server route schema.

And as @seanrathier pointed out:

Adoption in the CSP plugin may become cumbersome to follow the associated handlers, collocating them can give a visual clue as to what the handlers are mocking.

import { statusNotInstalled } from '../../../../server/routes/status/status.handlers.mock';
...
server.use(statusNotInstalled);

Also, the handlers specific for tests are close to the tests and the generic handlers that usually require only a one-time setup are stored as part of the mock server setup.

[opauloh]

After reflecting on it, Option 2 seems to me to be the best choice given the benefits of proximity and contextual relevance that Max and Sean pointed out

[maxcold]

for me Option 2 is also preferrable

[opauloh]

While syncing with @kfirpeled today, he pointed out a very good point about improving the readability for reviewers on tests

Let's suppose these two use cases:

 it('shows integrations installation prompt with installation links when integration is not-installed', async () => {
    server.use(statusHandlers.notInstalledHandler);
    renderNoFindingsStates();
    expect(screen.getByText(/loading/i)).toBeInTheDocument();

    await waitFor(() => {
      expect(
        screen.getByText(/detect security misconfigurations in your cloud infrastructure!/i)
      ).toBeInTheDocument();
    });

    await waitFor(() => {
      expect(screen.getByRole('link', { name: /add cspm integration/i })).toHaveAttribute(
        'href',
        '/app/fleet/integrations/cloud_security_posture-1.9.0/add-integration/cspm'
      );
    });

    await waitFor(() => {
      expect(screen.getByRole('link', { name: /add kspm integration/i })).toHaveAttribute(
        'href',
        '/app/fleet/integrations/cloud_security_posture-1.9.0/add-integration/kspm'
      );
    });
  });

  it('shows install agent prompt with install agent link when status is not-deployed', async () => {
    server.use(statusHandlers.notDeployedHandler);
    server.use(benchmarksHandlers.cspmInstalledHandler);
    renderNoFindingsStates();
    expect(screen.getByText(/loading/i)).toBeInTheDocument();

    await waitFor(() => {
      expect(screen.getByText(/no agents installed/i)).toBeInTheDocument();
    });

    await waitFor(() => {
      expect(screen.getByRole('link', { name: /install agent/i })).toHaveAttribute(
        'href',
        '/app/integrations/detail/cloud_security_posture-1.9.0/policies?addAgentToPolicyId=30cba674-531c-4225-b392-3f7810957511&integration=630f3e42-659e-4499-9007-61e36adf1d97'
      );
    });
  });

It is not imediate clear to the readers what are the meaning of every handler and what data it's mocking that impacts the test. And that requires checking others files to get the context of each handler, and having that extra work might increase the overhead and lose focus on the tests and it might slow down the readers. So @kfirpeled proposed that we keep the test more declarative by having exclusive handlers in the test itself.

In my opinion, while this is a little bit more verbose it's a fair trade as it also encourages us to keep the mocked responses short to what matters for the test, as for the example:

it('shows integrations installation prompt with installation links when integration is not-installed', async () => {
    mockGetRequest(STATUS_URL, {
      cspm: {
        status: 'not-installed',
      },
      kspm: {
        status: 'not-installed',
      },
    });
    renderNoFindingsStates();
    expect(screen.getByText(/loading/i)).toBeInTheDocument();

    await waitFor(() => {
      expect(
        screen.getByText(/detect security misconfigurations in your cloud infrastructure!/i)
      ).toBeInTheDocument();
    });

    await waitFor(() => {
      expect(screen.getByRole('link', { name: /add cspm integration/i })).toHaveAttribute(
        'href',
        '/app/fleet/integrations/cloud_security_posture-1.9.0/add-integration/cspm'
      );
    });

    await waitFor(() => {
      expect(screen.getByRole('link', { name: /add kspm integration/i })).toHaveAttribute(
        'href',
        '/app/fleet/integrations/cloud_security_posture-1.9.0/add-integration/kspm'
      );
    });
  });

  it('shows install agent prompt with install agent link when status is not-deployed', async () => {
    mockGetRequest(STATUS_URL, {
      cspm: {
        status: 'not-deployed',
      },
      kspm: {
        status: 'not-deployed',
      },
    });

    mockGetRequest(BENCHMARKS_URL, {
      items: [
        {
          package_policy: {
            id: '630f3e42-659e-4499-9007-61e36adf1d97',
            inputs: [
              {
                policy_template: 'cspm',
                enabled: true,
              },
            ],
          },
          agent_policy: {
            id: '30cba674-531c-4225-b392-3f7810957511',
          },
        },
      ],
    });

    renderNoFindingsStates();
    expect(screen.getByText(/loading/i)).toBeInTheDocument();

    await waitFor(() => {
      expect(screen.getByText(/no agents installed/i)).toBeInTheDocument();
    });

    await waitFor(() => {
      expect(screen.getByRole('link', { name: /install agent/i })).toHaveAttribute(
        'href',
        '/app/integrations/detail/cloud_security_posture-1.9.0/policies?addAgentToPolicyId=30cba674-531c-4225-b392-3f7810957511&integration=630f3e42-659e-4499-9007-61e36adf1d97'
      );
    });
  });

Now it's clear for the reader as it scans the code what is happening, and what is the part of the data that matters for that particular test.

For this I also suggested this abstraction called mockGetRequest so we don't have to avoid having to import http and HttpResponse from msw, or call server.use all the time.

cc @maxcold @seanrathier Let me know if you have any opinions on that, I adjusted the code with the latest changes

[maxcold]

@opauloh @kfirpeled tbh I don't like this approach and find it much more confusing. The assumption Now it's clear for the reader as it scans the code what is happening didn't work for me as it's much more clear to see benchmarksHandlers.cspmInstalledHandler rather than

mockGetRequest(BENCHMARKS_URL, {
      items: [
        {
          package_policy: {
            id: '630f3e42-659e-4499-9007-61e36adf1d97',
            inputs: [
              {
                policy_template: 'cspm',
                enabled: true,
              },
            ],
          },
          agent_policy: {
            id: '30cba674-531c-4225-b392-3f7810957511',
          },
        },
      ],
    });

This JSON doesn't tell me much by itself, while cspmInstalledHandler does. If I have all the context of the benchmarks handler implementation, I might know from a first glance that this json means "cspmInstalled", but imagine tens or handreds of tests with different combinations of the response. In my opinion, it will become a mess. It makes sense that when implementing a new endpoint, the implementer also creates first mocks abstracting different responses into use cases. And then these mocks are being reused across the code and when smth changes in the endpoint, it's simple to update the mocks as well. With the suggested approach my feeling is that we will have slightly different mocks all over the place. Eg. in your example for one test, it's necessary that agent_policy is in the response, for the second test where agent_policy is not needed another person still copies the mock with agent_policy in it (because we most likely won't be writing mocks each time from scratch, especially massive ones), for the third test a third person copies the mock but checks that agent_policy is not required and removes it from their mock. So in the end we have a spectrum of different mocks and approaches.

[opauloh]

Thanks for sharing your thoughts on that @maxcold .

What you stated here:

So in the end we have a spectrum of different mocks and approaches.

I agree this is something we should aim to avoid, or it can potentially make us circle back to the same stage we currently are with having to always create mocks (handlers in this case) for a test instead of aiming for mocking reuse.

I would suggest we stick with the server.use approach and start calibrating as we go, it might take some time to get adjusted to the handlers reuse approach, but the benefits of reusing can pottentially speed up things and make tests easier and faster as more handlers are added.

[seanrathier]

@kfirpeled 's suggestion gave me pause.

TLDR; we should provide a way for his suggestion and use option 2 for happy-path and complex handlers.

The broader spectrum of tests we would build would benefit from using option 2. For example, the findings status handler should have a response of cspm and kspm status as installed. This would be reused by multiple test suites such as the NoFindingsStates and Misconfigurations pages.

However, the NoFindingsStates page needs to test different combinations of statuses, so having a way to do what @kfirpeled suggested would be a good way to control the growth of mock handlers.

[seanrathier]

Thanks for bringing this closer the finish line! Just a few comments in the code.

Do you think we need a short problem statement in the summary?
Should we describe what we needed to be covered in FTRs that can now be covered with MSW?

Thoughts?

[seanrathier]

@maxcold you bring up a good point about collocation. It was not something we thought about during the POC, however, @opauloh may have while continuing the work.

I agree with your point.

Adoption in the CSP plugin may become cumbersome to follow the associated handlers, collocating them can give a visual clue as to what the handlers are mocking.

import { statusNotInstalled } from '../../../../server/routes/status/status.mock.handlers';
...
server.use(statusNotInstalled);

```

[seanrathier]

I am good with the overall addition of MSW, we can work out the details like naming conventions and handler locations later.

[maxcold]

kudos for pushing this forward @opauloh , LGTM!

[opauloh]

/ci

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: fb86ab6

Failed CI Steps

• FTR Configs #20

Test Failures

• [job] [logs] FTR Configs #20 / Cases Attachment framework Attachment hooks Modal filters with multiple selection

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
cloudSecurityPosture	681	682	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
cloudSecurityPosture	450.1KB	449.9KB	-174.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
cloudSecurityPosture	29	31	+2

Total ESLint disabled count

id	before	after	diff
cloudSecurityPosture	29	31	+2

History

• 💛 Build #216288 was flaky 58b7f98
• 💔 Build #216280 failed 58b7f98
• 💛 Build #214354 was flaky ce685e2
• 💛 Build #214051 was flaky fa62f50
• 💚 Build #213731 succeeded 3167393
• 💔 Build #213235 failed 26ecc3b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream