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

fix: logs not showing for test runs #4102

Merged
merged 25 commits into from
Mar 20, 2025
Merged

fix: logs not showing for test runs #4102

merged 25 commits into from
Mar 20, 2025

Conversation

Kiryous
Copy link
Contributor

@Kiryous Kiryous commented Mar 18, 2025

Closes #4093

AI Summary

  • Modified workflow execution model to support nullable workflow_ids for test runs via new database migration
  • Unified workflow result handling in WorkflowManager by having both test and non-test runs use _save_workflow_results
  • Added proper event handling for manual test runs through new get_event_from_body function
  • Enhanced WorkflowExecutionResults component to handle unsaved workflow test runs using workflowYaml prop
  • Improved error handling and status tracking for workflow test executions with better logging

Before

Screen.Recording.2025-03-20.at.12.12.43.mov

After

Screen.Recording.2025-03-20.at.12.31.49.mov

NOTE: The alert.prop is not in context will be fix in a separate PR.

Copy link

vercel bot commented Mar 18, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
keep ⬜️ Ignored (Inspect) Visit Preview Mar 20, 2025 9:56am

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 15.78947% with 16 lines in your changes missing coverage. Please review.

Project coverage is 30.96%. Comparing base (d667aaf) to head (fe82a73).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
keep/workflowmanager/workflowscheduler.py 11.11% 8 Missing ⚠️
keep/parser/parser.py 0.00% 4 Missing ⚠️
keep/workflowmanager/workflowmanager.py 0.00% 2 Missing ⚠️
keep/api/core/db.py 0.00% 1 Missing ⚠️
keep/workflowmanager/workflow.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4102       +/-   ##
===========================================
- Coverage   47.03%   30.96%   -16.07%     
===========================================
  Files         165       92       -73     
  Lines       16210    10155     -6055     
===========================================
- Hits         7625     3145     -4480     
+ Misses       8585     7010     -1575     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Kiryous
Copy link
Contributor Author

Kiryous commented Mar 18, 2025

@greptileai

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses test run functionality in the Keep platform by making workflow executions more robust and properly saving test run results. Here's a concise summary of the key changes:

  • Modified workflow execution model to support nullable workflow_ids for test runs via new database migration
  • Unified workflow result handling in WorkflowManager by having both test and non-test runs use _save_workflow_results
  • Added proper event handling for manual test runs through new get_event_from_body function
  • Enhanced WorkflowExecutionResults component to handle unsaved workflow test runs using workflowYaml prop
  • Improved error handling and status tracking for workflow test executions with better logging

The changes appear well-structured and properly tested, focusing on fixing the core issue of test runs not being saved while maintaining consistency between test and regular workflow executions.

18 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

@Kiryous Kiryous marked this pull request as ready for review March 19, 2025 16:35
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. Bug Something isn't working labels Mar 19, 2025
@Kiryous
Copy link
Contributor Author

Kiryous commented Mar 19, 2025

@greptileai

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The changes focus on enhancing test workflow execution handling in the Keep platform, particularly around test run management and execution tracking.

  • Added new test cases in /tests/test_workflowmanager.py to verify manual event workflow handling for both regular and test runs
  • Modified /keep/workflowmanager/workflowscheduler.py to support nullable workflow IDs and added test_run flag in execution records
  • Enhanced logging around test run execution status and error handling
  • Improved workflow execution tracking by properly distinguishing between test and regular workflow runs

2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

@Kiryous Kiryous changed the title fix: test run not saved workflows fix: logs not showing for test runs Mar 20, 2025
Copy link
Contributor

@VladimirFilonov VladimirFilonov left a comment

Choose a reason for hiding this comment

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

LGTM

@Kiryous Kiryous merged commit 2ba2d9e into main Mar 20, 2025
18 checks passed
@Kiryous Kiryous deleted the fix/4093-test-run branch March 20, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Remember to wrap your app in a <CopilotKit> on Test Run
3 participants