Skip to content

tests: add unit tests for DAG builder#230531

Merged
skynetigor merged 30 commits intoelastic:mainfrom
skynetigor:13412-Add-unit-tests-for-DAG-builder
Aug 6, 2025
Merged

tests: add unit tests for DAG builder#230531
skynetigor merged 30 commits intoelastic:mainfrom
skynetigor:13412-Add-unit-tests-for-DAG-builder

Conversation

@skynetigor
Copy link
Contributor

@skynetigor skynetigor commented Aug 5, 2025

Summary

closes: #13412

  • Introduce AtomicGraphNode
  • Add unit-tests for converting If condition step into DAG
  • Add unit-tests for converting Foreach step into DAG
  • Tests for other steps have to be done in along with step implementation

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
  • Review the backport guidelines and apply applicable backport:* labels.

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

…onBranchNodeImpl

- Added validation to ensure EnterIfNodeImpl only has 'enter-condition-branch' successors.
- Implemented error handling in ExitConditionBranchNodeImpl to check for 'exit-if' successor type.
- Updated tests to cover new validation scenarios for both node implementations.
@skynetigor skynetigor changed the title 13412 add unit tests for dag builder tests: add unit tests for DAG builder Aug 5, 2025
@skynetigor skynetigor marked this pull request as ready for review August 5, 2025 10:59
@skynetigor skynetigor requested a review from a team as a code owner August 5, 2025 10:59
@skynetigor skynetigor added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Aug 5, 2025
@skynetigor skynetigor added the Team:One Workflow Team label for One Workflow (Workflow automation) label Aug 5, 2025
@skynetigor skynetigor requested a review from Copilot August 5, 2025 16:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive unit tests for the DAG builder component, which converts workflow steps into executable graphs. The changes primarily focus on testing the conversion logic for conditional (If) and loop (Foreach) steps, along with atomic steps.

  • Add unit tests for converting atomic, If condition, and Foreach steps into DAG structures
  • Refactor the DAG building logic to properly handle atomic steps with a new AtomicGraphNode type
  • Update function signatures to accept WorkflowSchema instead of WorkflowExecutionEngineModel

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
build_execution_graph.test.ts Comprehensive test suite covering atomic, if, foreach, and complex workflow scenarios
build_execution_graph.ts Refactored to support atomic step nodes and improved step visiting logic
workflow_task_runner.ts Updated to pass workflow.definition.workflow to graph converter
scheduler_service.ts Updated to pass workflow.definition.workflow to graph converter
step_factory.ts Added support for atomic step implementation and removed fallback to connector step
atomic_step_impl.ts New implementation for atomic workflow steps
base.ts Added AtomicGraphNode schema and type definitions
Various type files Updated exports to include new AtomicGraphNode type

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/workflows 154 156 +2
Unknown metric groups

API count

id before after diff
@kbn/workflows 155 157 +2

History

@skynetigor skynetigor enabled auto-merge (squash) August 6, 2025 10:41
Copy link
Contributor

@Kiryous Kiryous left a comment

Choose a reason for hiding this comment

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

LGTM

@skynetigor skynetigor merged commit c2406c8 into elastic:main Aug 6, 2025
12 checks passed
gergoabraham pushed a commit to gergoabraham/kibana that referenced this pull request Aug 7, 2025
## Summary

- Introduce AtomicGraphNode
- Add unit-tests for converting If condition step into DAG
- Add unit-tests for converting Foreach step into DAG
- Tests for other steps have to be done in along with step
implementation


### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [ ] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...
@wildemat wildemat mentioned this pull request Aug 7, 2025
10 tasks
NicholasPeretti pushed a commit to NicholasPeretti/kibana that referenced this pull request Aug 18, 2025
## Summary

- Introduce AtomicGraphNode
- Add unit-tests for converting If condition step into DAG
- Add unit-tests for converting Foreach step into DAG
- Tests for other steps have to be done in along with step
implementation


### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [ ] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:One Workflow Team label for One Workflow (Workflow automation) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants