Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CircleCI Test Split Feature #75

Closed
wants to merge 182 commits into from
Closed

Add CircleCI Test Split Feature #75

wants to merge 182 commits into from

Conversation

vanditaMW
Copy link
Collaborator

@vanditaMW vanditaMW commented Aug 14, 2024

Closing this PR since there is a change in the design. I have raised a new PR which has the updated design

Added the "split-type" parameter to enable the CircleCI Test Split feature within the "run-tests" command. The "split-type" parameter will accept inputs such as "filename," "filesize," and "timings" to perform the CircleCI Test Split functionality. Unit tests were added, and the interactive workflow was reviewed to ensure proper integration.

image

For more details, refer to the Confluence page below:
https://mathworks.atlassian.net/wiki/spaces/CI/pages/57271792/D+Support+Test+Splitting+in+CircleCI

@mw-jsgeorge mw-jsgeorge self-assigned this Aug 16, 2024
@vanditaMW vanditaMW requested a review from mw-kapilg August 16, 2024 04:36
Copy link
Member

@mw-kapilg mw-kapilg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a few comments for you to have a look at. Thanks Vandit!

.circleci/test-deploy.yml Show resolved Hide resolved
src/commands/run-tests.yml Show resolved Hide resolved
src/commands/run-tests.yml Show resolved Hide resolved
src/scripts/run-tests.sh Outdated Show resolved Hide resolved
getCircleCISplitFiles.m Outdated Show resolved Hide resolved
@vanditaMW vanditaMW requested a review from mw-hrastega August 16, 2024 08:27
@mcafaro
Copy link
Member

mcafaro commented Aug 16, 2024

I think it's worth discussing this design some more. The select-by-filename param (shown below) feels more flexible and broadly applicable across all of our integrations than a split-type param.

- matlab/run-tests:
    select-by-filename: $(circleci tests glob 'tests/**/*.m' | circleci tests split --split-by=timings)
    test-results-junit: results

With select-by-filename, you can use any test splitter you want and we would be able to put the same param on our "run tests" steps in GitHub Actions, Azure Pipelines, Jenkins, Bamboo, etc. Not to mention, select-by-filename has uses beyond test splitting.

With split-type, we're tying ourselves tightly to a CircleCI-specific feature and implementation. We're baking in knowledge of CircleCI to genscript, which is really meant to be a platform agnostic system, and we're producing a lot of logic and doc here that we can't share across our family of integrations. In addition, there are other controls on CircleCI's splitter like --timings-type and --total and --index, that we're not allowing the user to configure.

@mw-hrastega
Copy link
Member

I think it's worth discussing this design some more. The select-by-filename param (shown below) feels more flexible and broadly applicable across all of our integrations than a split-type param.

- matlab/run-tests:
    select-by-filename: $(circleci tests glob 'tests/**/*.m' | circleci tests split --split-by=timings)
    test-results-junit: results

With select-by-filename, you can use any test splitter you want and we would be able to put the same param on our "run tests" steps in GitHub Actions, Azure Pipelines, Jenkins, Bamboo, etc. Not to mention, select-by-filename has uses beyond test splitting.

With split-type, we're tying ourselves tightly to a CircleCI-specific feature and implementation. We're baking in knowledge of CircleCI to genscript, which is really meant to be a platform agnostic system, and we're producing a lot of logic and doc here that we can't share across our family of integrations. In addition, there are other controls on CircleCI's splitter like --timings-type and --total and --index, that we're not allowing the user to configure.

I agree that we need a naming pass for the new parameter. I prefer split-by over split-type because the former resonates more with users of CircleCI test splitting.

@@ -40,6 +40,11 @@ parameters:
parameter requires a Parallel Computing Toolbox license.
type: boolean
default: false
split-type:
description: >
Option to use CircleCI Test Split feature, specified as 'timings', 'filename' or 'filesize'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use the same terminology as in CircleCI documentation. Instead of "CircleCI Test Split feature" CircleCI uses "CircleCI test splitting" in their documentation: https://circleci.com/docs/parallelism-faster-jobs/#:~:text=CircleCI%20test%20splitting

The information in this description is minimal. For example, it doesn't say anything about the parallelism key. At a minimum, we should link to this page: https://circleci.com/docs/parallelism-faster-jobs/. Alternatively, the description can be expanded to include some important details.

Also, it might be good to clarify how one should use the new parameter alongside use-parallel.

@vanditaMW
Copy link
Collaborator Author

vanditaMW commented Aug 19, 2024

I think it's worth discussing this design some more. The select-by-filename param (shown below) feels more flexible and broadly applicable across all of our integrations than a split-type param.

- matlab/run-tests:
    select-by-filename: $(circleci tests glob 'tests/**/*.m' | circleci tests split --split-by=timings)
    test-results-junit: results

With select-by-filename, you can use any test splitter you want and we would be able to put the same param on our "run tests" steps in GitHub Actions, Azure Pipelines, Jenkins, Bamboo, etc. Not to mention, select-by-filename has uses beyond test splitting.

With split-type, we're tying ourselves tightly to a CircleCI-specific feature and implementation. We're baking in knowledge of CircleCI to genscript, which is really meant to be a platform agnostic system, and we're producing a lot of logic and doc here that we can't share across our family of integrations. In addition, there are other controls on CircleCI's splitter like --timings-type and --total and --index, that we're not allowing the user to configure.

Hey @mcafaro,
I appreciate your perspective on the flexibility and broad applicability of the above proposed design across various integrations.

Following up on our conversation, we identified a couple of limitations with the above implementation:

  1. The "split-by-timing" feature won't function as expected due to limitations in the JUnit XML file generated from our plugin when using the statement $(circleci tests glob 'tests/**/*.m' | circleci tests split --split-by=timings).

  2. The above design also necessitates the use of the HasFileName selector, which is implemented internally. This is due to the complexities associated with filenames, such as handling Simulink Tests, test classes with methods spanning multiple files, methods defined in superclasses, and (future) tests defined dynamically that aren't located in a single file.

@mcafaro
Copy link
Member

mcafaro commented Aug 19, 2024

@vanditaMW Here is the same effect achieved with a select-by-name parameter:

Split alphabetically:

- matlab/run-tests:
    select-by-name: $(circleci tests glob 'tests/**/*.m' | circleci tests split | awk -F'[/.]' '{print $(NF-1) "/*"}')

Split by file size:

- matlab/run-tests:
    select-by-name: $(circleci tests glob 'tests/**/*.m' | circleci tests split --split-by=filesize | awk -F'[/.]' '{print $(NF-1) "/*"}')

Split by timings:

- matlab/run-tests:
    select-by-name: $(circleci tests glob 'tests/**/*.m' | awk -F'[/.]' '{print $(NF-1)}' | circleci tests split --split-by=timings | awk '{print $0 "/*"}')

This can be implemented without a new selector or JUnit modifications.

No doubt the commands are complex but an example of each can be added to our CircleCI doc. The command complexity is also consistent with other languages where CircleCI provides examples.

Copy link
Member

@nbhoski nbhoski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanditaMW you could resolve the conversation for ease of review

.circleci/test-deploy.yml Show resolved Hide resolved
generateFolderSelectionStatement.m Show resolved Hide resolved
generateFolderSelectionStatement.m Show resolved Hide resolved
.circleci/test-deploy.yml Show resolved Hide resolved
@vanditaMW vanditaMW closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants