[Cloud Security] Refactor tests and move tests into single folder

Omolola-Akinleye commented 9 months ago

Motivation:

We want to create a well-organized E2E project structure and common utilities that can help the efficiency and maintenence for Cloud Security Posture Plugin FTRS.

DOD: [ ] Create a refactoring plan for Cloud security E2E project structure [ ] Create a folder for each type of FTR (ie. functional, api_integration, cypress) [ ] Establish a folder structure pattern for each FTR ( utils, commons, configs, services, mocks etc.) [ ] Update readme and documentation on E2E project structure

maxcold commented 8 months ago

@Omolola-Akinleye can you add more details to this ticket? Which tests do you mean here?

Omolola-Akinleye commented 8 months ago

@Omolola-Akinleye can you add more details to this ticket? Which tests do you mean here?

@maxcold The task here is to move test directories x-pack/test/cloud_security_posture_api and x-pack/test/api_integration/apis/cloud_security_posture to the cloud security functional single folder - x-pack/test/api_integration/apis/cloud_security_posture

Omolola-Akinleye commented 5 months ago

After chatting with @kfirpeled, we want a few suggestions, how to organize e2e Project Structure. After some research, I would like to proposed two principles consistent structure and mental framework for developers to find, identify, and reuse code and files easily.

The following principles LIFT and LINT:

*L.I.F.T

Locate: "Place files where you and others can easily find them."
Identify: "Name files and directories clearly to know what they contain at a glance."
Flatten: "Use a flat structure for as long as practical to simplify navigation."
T-DRY (Try to be DRY): "Reuse code where possible, but not at the expense of readability or simplicity."

*L.I.N.T

Locate: "Ensure files are intuitively placed for quick access."
Identify: "Use descriptive names for immediate understanding of contents."
Nested: "Group related files logically within structured directories."
T-DRY: "Aim for code reuse to avoid duplication, keeping clarity in focus."

Below are suggestions:

Flatten Example Using LIFT

- `x-pack/test/`
  - `csp_package_functional/`      # Locate: Tests are easily found at the top level.
  - `csp_package_api_integration/` # Identify: Descriptive names indicate test coverage.
  - `csp_basic_api_integration/`   # Flatten: Keeping the structure flat for easy scanning.
  -  `common/utils/cloud_security_posture/`

Locate & Identify: Direct and descriptive, facilitating quick file location and understanding.
Flatten: No unnecessary nesting, ensuring ease of navigation.
T-DRY: Not explicitly demonstrated here but implies avoiding unnecessary repetition across test suites.

Parent with Flatten Example Using Both LINT and LIFT

LIFT:

- `x-pack/test/cloud_security_posture/`
  - `csp_package_functional/`      # Locate & Identify: Clearly named and logically placed. We could also search names IDE quickly
  - `csp_package_api_integration/` # Flatten: Flat within the parent, easy to identify and navigate from one folder to another
  - `csp_basic_api_integration/` 
  - `utils/` # T-DRY: One folder to placing utilities where it makes sense

  # Configs Location
  - x-pack/test/cloud_security_posture/csp_package_functional/config.ts # Locate & Identify each config per FTR directory easily
  - x-pack/test/cloud_security_posture/csp_package_api_integration/config.ts
  - x-pack/test/cloud_security_posture/csp_basic_api_integration/config.ts

Locate & Identify: Grouping under a meaningful parent directory with clear names
Flatten: Maintains a flat hierarchy within cloud_security_posture for simplicity.
T-DRY: Sharing common setups or utilities where applicable, without overcomplicating. Als

LINT:

- `x-pack/test/cloud_security_posture/`
  - `csp_package_functional/`       # Nest: Logically grouped under the parent directory.
  - `csp_package_api_integration/`  # T-DRY: Utilize shared configurations and helpers to avoid repetition.
  - `csp_basic_api_integration/`
  - `utils/` # T-DRY: One folder to placing utilities where it makes sense

Nest: Grouping related tests under a common parent implies logical organization.
T-DRY: Applied by reusing configurations and helpers to maintain DRY principles without sacrificing readability or structure clarity.

Parent with Nested Example Using LINT (Focusing on T-DRY)

- `x-pack/test/cloud_security_posture/`
  - `functional/`
    - `package/`                  # Nest: Deeper organization 
  - `api_integration/`
    - `basic/`                    # Identify: Clearly defines the test scope.
    - `package/`                 
    - `utils` # T-DRY: Shared utilities for API integration tests to reduce redundancy.

Nest & Identify: Carefully structured to both organize and clearly define test scopes.
T-DRY: Emphasized through shared utilities and configurations, particularly within nested structures, to minimize redundancy while keeping tests understandable and easy to manage.

LIFT - Flatten

Pros

Simplicity and Speed: L.I.F.T encourages a structure that's easy to navigate and quick to understand, making it ideal for smaller projects or those emphasizing rapid development.
Direct Access: With an emphasis on flattening, files are generally fewer clicks away, streamlining workflows.
Clear Naming: The Identify principle ensures that file and directory names clearly convey their contents or purpose. Adding prefix csp_ csp_package will quickly identify different and also search FTRS

Cons

Scalability Issues: As projects grow, a flat structure might become cluttered, making it harder to manage and navigate. If we begin to 7 api integration FTR setups, readability would be difficult due to The Magical Number Seven, Plus or Minus Two rule.
Potential for Redundancy: While T-DRY encourages avoiding repetition, the flattened nature might lead to similar files scattered across the project, raising the importance of diligent naming and organization.

LINT - Nested

Pros

Enhanced Organization: By nesting related files, L.I.N.T facilitates a more organized and hierarchical structure, ideal for larger project codebase.
Scalability: A nested approach can more easily accommodate project growth, keeping the structure tidy and manageable even as new elements are added.
Focused Modularity: Grouping related functionalities makes it easier to understand the project's modular structure and fosters code reuse within those modules.

Cons

Increased Complexity: More levels in the folder hierarchy can make navigation more cumbersome, potentially slowing down file access and increasing the learning curve for new team members. For instance, importing paths
Design Overhead: Requires careful planning to design an intuitive and efficient hierarchy, which might need adjustments as the project evolves and grows. For instance, How do we design project structure now to include CypresS POC.

Omolola-Akinleye commented 5 months ago

My prosposal is the Parent Structure with flatten structure.

Pros:

Commons Folder Accessibility: Positioned at the root, it grants straightforward access across different Feature Test Runner (FTR) folders, housing utilities and shared configurations for packages. This centralization facilitates resource sharing for utils, helpers.

Centralized Parent Structure: Offers a singular access point for FTRs (e.g., API integration, API integration with package installation, functional testing with package installation), enhancing organizational coherence over dispersed structures.

Flattened Hierarchy: Simplifies navigation among FTR directories and improves the accessibility of the Commons directory for helper test code or types. This approach promotes more cognitive efficiency, optimizing for the "Magical Number Seven, Plus or Minus Two" in memory and attention, thereby streamlining onboarding and reducing the cognitive burden of nested folders. More on The Magical Number Seven, Plus or Minus Two.

Prefixing for Clarity: If we want prefix test folders, this will help team members with context to quickly identify FTRs purpose. Another benefit, better IDE experience in terms searching and navigating with quicker and few search results.

Consistency with Kibana: Aligns with Kibana's monolithic architecture, fostering ease of navigation and cross-team learning opportunities within a unified testing framework.

Cons:

Scalability Concerns: A flat structure could hinder quick identification as the number of API Integration folders grows. A nested approach might offer better organization and scalability, particularly with anticipated expansions such as CDR implementation.

Prefix Redundancy: The naming convention may introduce unnecessary repetition, impacting clarity.

Lack of Logical Grouping: Absence of a structured hierarchy for related tests could impede the efficiency of locating specific test scopes (API, functional, Cypress).

Alternative Consistent Structure: Maintaining a flattened structure while relocating common utilities or test code (e.g., x-pack/test/common/utils/security_solution) might align better with existing E2E test folder practices across teams, promoting uniformity.

maxcold commented 5 months ago

@Omolola-Akinleye thanks for the very detailed proposal, well described and reasoned! My 5 cents:

I'd personally vote for the Parent with Nested structure for the reason that this structure the test_serverless folder has. I think following the as close as possible the naming convention and the folder structure would remove some cognitive overhead when working with both ESS and Serverless tests
Not exactly the structure question but is there any reason we want to keep two directories for api_integration? I mean the basic suit doesn't tell much about how basic is different from not basic. Can we just refactor the basic tests into the suits that can run just with the same config that other api_integration tests run? As a first step we ofc can keep these two configs separate and have this basic folder, but longer term I don't see much sense in it as it is confusing. I wouldn't know what test should go into basic.

Omolola-Akinleye commented 5 months ago

I'd personally vote for the Parent with Nested structure for the reason that this structure the test_serverless folder has. I think following the as close as possible the naming convention and the folder structure would remove some cognitive overhead when working with both ESS and Serverless tests

@maxcold Thanks Max! That's a great point. I agree and if we go with the Parent with nested structure, then we would have to apply the same structure to the test_serverless folder. I only saw API integration tests without package FTRs. I was still confused about the convention of the serverless since serverless tests are fewer and less mature than ess. Should we still use the same page objects, utils, and configs from ESS?

Not exactly the structure question but is there any reason we want to keep two directories for api_integration? I mean the basic suit doesn't tell much about how basic is different from not basic. Can we just refactor the basic tests into the suits that can run just with the same config that other api_integration tests run? As a first step we ofc can keep these two configs separate and have this basic folder, but longer term I don't see much sense in it as it is confusing. I wouldn't know what test should go into basic.

So basic_api_integration is the general base API Integration FTR config and package_api_integration which needs the CSP package to be setup in the config. I did try the approach of combining both configs when I talked to the Appex QA team they mentioned it would be costly when running tests and cause more flaky errors in the CI jobs so it's better to keep api-integration test configs separate. Here is a slack-threadfor more context.

Ticket-Update - I moved this ticket todo and on hold until after the discussion in our huddle. There are a lot of open questions and I would like to propose suggestions in our next huddle meeting and get feedback from the team before continuing to refactor.

cc: @kfirpeled

maxcold commented 5 months ago

@Omolola-Akinleye

I only saw API integration tests without package FTRs.

If I understand you correctly, the test_serverless has both api_integrations and functional folders at the top level, they separate API integration and FTRs tests

I was still confused about the convention of the serverless since serverless tests are fewer and less mature than ess. Should we still use the same page objects, utils, and configs from ESS?

It is not recommended to use utilities, page objects, and configs from ESS tests for Serverless. The platform team is pushing for having serverless as independent as possible. So for this issue, I'd leave Serverless tests out of the scope, would just consider their folder structure for consistency.

Thanks for providing more details about basic API tests!

Omolola-Akinleye commented 5 months ago

Current pain points and mind map for FTRS

maxcold commented 5 months ago

following up on the discussion on the Team Huddle. To combine basic and package API tests we could move them to one folder, just have two config.js files, eg. config.basic.js and config.csp_package.js or smth and specify in each config which test suits should run with this config file via testFiles which we already use in x-pack/test/cloud_security_posture_api/config.ts All in all I agree that having API test suits colocated is not our biggest issue, if we move them closer, but still separated by some basic/package dir, it will already be an improvement

kfirpeled commented 5 months ago

Joining @maxcold with his suggestion and approving Lola's recommendation (I hope I got it right) Having a parent folder under tests would look something like:

x-pack/test/cloud_security_posture
├── api
│   ├── routes
│   ├── config.base.ts
│   └── config.package.ts
├── functional
│   ├── pages
│   ├── config.base.ts
│   └── config.package.ts
└── common
    └── utils.ts
x-pack/test_serverless
├── api/test_suites/security/cloud_security_posture
└── functional/test_suites/security/ftr/cloud_security_posture

And @Omolola-Akinleye a friendly note to pay attention when you change things around, what determines the entry points is the following config file: https://github.com/elastic/kibana/blob/main/.buildkite/ftr_configs.yml

And I found an example of different config files based on use-cases, similar to what we try to achieve in the security solution:

Omolola-Akinleye commented 5 months ago

@kfirpeled Looks good and got it! Yes I'm familar configs entry list in kibana/blob/main/.buildkite/ftr_configs.yml

maxcold commented 5 months ago

The only thing is that I wouldn't touch serverless tests. I believe the expectation is that the separation between ESS and serverless tests are on the root level with 'tests' and 'tests_serverless'. Tbh I expext a lot of obstacles and confusion if we try to colocate these tests

Omolola-Akinleye commented 5 months ago

Yes agreed. I wouldn't touch the serverless test for this ticket. I think we can simply update documentation on where all of the tests are located from kibana/blob/main/.buildkite/ftr_configs.yml

kfirpeled commented 5 months ago

@elastic/kibana-cloud-security-posture last chance to add any comments till the end of Tuesday, before we will take the final decision

elastic / kibana