[Security Solution] POC reusing types and avoiding data-driven

[MadameSheema]

Summary

This PR is a little POC about reusing existing types and trying to avoid data-driven inside the tests, as part of the on-week project.

Please note the following:

• We have refactored just one test Creates and enables a new rule for Custom query rules
• If this PR is finally merged, we could have some duplicated code till the rest of the tests are properly refactored.
• In some methods like fillSeverity we are not updating the types yet to avoid breaking the current tests. That would be changed as well

Reusing existing types

We started having custom types and interfaces in Cypress in order to:

• Be independent of the application
• To have a "structure" of data that represents the data the user needs to fill on the UI.
  As the suite has grown, so have grown the types and interfaces. In the specific case of the interfaces we have ended up with a weird mix of values needed in both UI and API not representing anymore what we originally wanted and making the current tests harder to maintain and expand over time. Mostly because with the interfaces we have know we are duplicating a lot of information.

The overall idea now is to remove all the custom-created types and interfaces and reuse the ones we already have in our application as well as just created small pieces of mock data that would be used for each one of the tests when needed.

Avoid data-driven inside the tests.

When the rule creation tests originally were implemented, the idea was for the methods that interacts with the UI to hide the implementation/logic of the fulfillment of the different sections with the aim of making the tests easier to extend and maintain.

This idea worked at the beginning when we just had a few rules and the logic was more simple than the one we have now. Currently this model has turned the other way around making the development of new tests and the maintenance of the current ones more complex.

Taking advantage of the new types, we are creating also small pieces of mocked data that we are using to fill just what we need/want.

Next steps

• Once the new structure is agreed, continue the refactor with the different type of rules
• Agree if we need to fill ALL the fields for each one of the different creation rule tests
• For the methods that return the mock data, agree if we want to use the schema naming or the one we are seeing on the UI i.e. getNote() vs getInvestigationGuide
• For the methods that fill the different sections, agree if we want to have default data as the current refactor or pass it explicitly

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[MadameSheema]

@elasticmachine merge upstream

[maximpn]

@MadameSheema the changes look good, it's a good improvement.

Though there is a comment for an improvement.

[maximpn]

@MadameSheema is there a special point in using parameterless functions for the default test data? As far as I see such functions can be replaced with constants, for example for getQuery it will be

export const DEFAULT_QUERY = 'host.name: *' as const;

The name DEFAULT_QUERY can vary but should represent the intent. Which also can be just QUERY.

[banderror]

SOME_QUERY might make sense when you don't care about the actual value.

[banderror]

Looks a bit raw, but I think this is the right direction. LGTM as the first step towards it 👍 Many thanks @MadameSheema!

[banderror]

In a test, normally you'd need many of these, which will lead to long import statements and inferior DX (every tiny bit is not available until you import it).

import {
  getDefaultIndexPatterns,
  getDescription,
  getFalsePositives,
  getFrom,
  getInterval,
  getQuery,
  getReferenceUrls,
  getRiskScore,
  getRuleName,
  getSeverity,
  getTags,
  getThreat,
  getThreatSubtechnique,
  getThreatTechnique,
} from '../../data/detection_engine';

It's not a production code and we don't need tree-shaking here => we could combine all these functions in an object to make it easier to use them. Something like that:

export const ruleFields = {
  getName(): Name {
    return 'Test Rule';
  },

  getDescription(): Description {
    return 'The rule description';
  },

  // etc
};

import { ruleFields } from '../../objects/rule_fields';

[MadameSheema]

Love it! Thanks for the suggestion.

[banderror]

This is probably an off-topic, because it seems like you kept the previous behavior of the test, but I think in a basic e2e test we should be filling in regular (non-saved) query.

Same for severity, risk score, and other defaultable or optional fields below.

I'd have one basic e2e test that fills in minimal (and only required) fields, maybe another one that fills in all the fields including optional and defaultable, and potentially a few special tests (like one for saved queries).

[MadameSheema]

Yes, I kept the previous behavior, I listed it as a Next steps Agree if we need to fill ALL the fields for each one of the different creation rule tests. We can chat about it (although I believe we are on the same page). And do it in a follow-up PR to not compromise the current coverage we might have.

[banderror]

And do it in a follow-up PR to not compromise the current coverage we might have.

Sure, there's no rush with that 👍

[banderror]

Where this query is coming from? This assertion is brittle, because if somewhere under the hood it starts using a different query, this will break with a false negative result.

Please do something like that:

const query = getQuery();
// ...
fillQuery(query);
// ...
cy.get(CUSTOM_QUERY_INPUT).should('have.value', query);

[banderror]

Same here and other similar places in this test.

const ruleName = getRuleName();
// ...
fillRuleName(ruleName);
// ...
cy.get(RULE_NAME_INPUT).invoke('val').should('eql', ruleName);

Sometimes you don't really care about exact value that will be filled in. Like here, you probably care that some rule name will be filled, and then whatever was filled will be shown when you switch back to the About step. In this case the code could be simplified this way:

export const fillRuleName = (ruleName: string = getRuleName()) => {
  cy.get(RULE_NAME_INPUT).clear({ force: true }).type(ruleName, { force: true });
  return ruleName;
};

const ruleName = fillRuleName();
// ...
cy.get(RULE_NAME_INPUT).invoke('val').should('eql', ruleName);

[banderror]

Sorry off-topic again, but I think this test does too much which is irrelevant to rule creation functionality.
In order to check if the rule was created properly it should be sufficient to

1. fetch the created rule via the API and assert its fields
2. validate that after creation the user is navigated to the rules page
3. and the rules table is refreshed and shows the newly created rule

Rule Details page is a separate feature that should be covered by separate e2e tests.

[banderror]

I'd group these into an object as well which would improve readability, encapsulate details and simplify imports:

aboutStep.fillRuleName();
aboutStep.fillDescription();
// etc

[vitaliidm]

Thanks for improvements @MadameSheema

[vitaliidm]

Regarding reusing already defined types: types imported here are defined for the values that are used on API level. I.e. ones that are used when rules are created/updated through API calls.

But the functions defined here are used for filling control values in form, which can not be the same type
This leads to a situation like this, where risk scores have to be converted to a string type, which can be consumed by the cypress method.

export const fillRiskScore = (riskScore: string = getRiskScore().toString()) => {
  cy.get(DEFAULT_RISK_SCORE_INPUT).type(`{selectall}${riskScore}`, { force: true });
};

It might be ok though, just something that caught my eye

[MadameSheema]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: d8ab9e9

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
osquery	1	2	+1

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	19	21	+2
fleet	59	65	+6
osquery	108	113	+5
securitySolution	441	447	+6
total			+19

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	20	22	+2
fleet	67	73	+6
osquery	109	115	+6
securitySolution	518	524	+6
total			+20

History

• 💔 Build #88360 failed 5e9a256
• 💚 Build #86976 succeeded 604ce5e
• 💚 Build #81655 succeeded 5a48703
• 💚 Build #80932 succeeded 8101d34
• 💛 Build #80476 was flaky fa6bdaf

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

cc @MadameSheema

[MadameSheema]

💚 All backports created successfully

Status	Branch	Result
✅	8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation