-
Notifications
You must be signed in to change notification settings - Fork 0
Implement consolidated summary and failed example reporting in orchestrator #27
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
base: v0.1.0
Are you sure you want to change the base?
Conversation
Co-authored-by: vovka <127781+vovka@users.noreply.github.com>
Co-authored-by: vovka <127781+vovka@users.noreply.github.com>
…estrator Co-authored-by: vovka <127781+vovka@users.noreply.github.com>
Co-authored-by: vovka <127781+vovka@users.noreply.github.com>
There was a problem hiding this 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 enhances the orchestrator and formatter to collect detailed failure data from parallel processes, send structured summaries via IPC, and render a consolidated report with aggregated statistics and a graceful timeout for missing summaries.
- Formatter now tracks failed examples, pending counts, start time, and sends a
:summaryIPC message with structured data. - Orchestrator listens for
summarymessages, aggregates results, and renders a combined summary (failures list, totals, wall-clock vs. process times). - Tests added for summary handling across processes and timeout behavior, plus formatter summary IPC notification.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/parallel_matrix_formatter/orchestrator_spec.rb | Added tests for multi-process summary collection and missing summary timeout |
| spec/parallel_matrix_formatter/formatter_spec.rb | Updated formatter tests to verify structured summary IPC messages |
| spec/integration/parallel_matrix_formatter_output.txt | Updated integration output to show detailed failures and aggregate summary |
| lib/parallel_matrix_formatter/orchestrator.rb | Implemented summary message handling, aggregation, timeout, and rendering logic |
| lib/parallel_matrix_formatter/formatter.rb | Enhanced formatter to collect failures, pending count, start time, and send structured summary |
Comments suppressed due to low confidence (2)
spec/parallel_matrix_formatter/orchestrator_spec.rb:243
- It would be valuable to add a test where there are zero failures but some pending examples (e.g., '5 examples, 0 failures, 2 pending') to verify that pending counts display correctly when failures are absent.
expect(output).to have_received(:puts).with("8 examples, 2 failures, 1 pending")
lib/parallel_matrix_formatter/orchestrator.rb:74
- Because
startimmediately renders once all summaries arrive andclosealso callswait_for_summarieswhich renders again, you may see the consolidated summary printed twice. Consider removing the immediate render instartand deferring all summary printing to the close path.
render_consolidated_summary if all_summaries_received?
| @output.puts "\n" | ||
|
|
||
| # Print failed examples | ||
| unless all_failed_examples.empty? | ||
| @output.puts "Failures:" | ||
| @output.puts | ||
| all_failed_examples.each_with_index do |failure, index| | ||
| @output.puts " #{index + 1}) #{failure['description']}" | ||
| @output.puts " #{failure['location']}" if failure['location'] | ||
| @output.puts " #{failure['message']}" if failure['message'] | ||
| @output.puts " #{failure['formatted_backtrace']}" if failure['formatted_backtrace'] | ||
| @output.puts |
Copilot
AI
Jul 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Printing an explicit newline here and then additional blank lines in the loop can lead to inconsistent spacing. Consider consolidating newline logic (e.g., one blank puts before failures) for clearer formatting.
| @output.puts "\n" | |
| # Print failed examples | |
| unless all_failed_examples.empty? | |
| @output.puts "Failures:" | |
| @output.puts | |
| all_failed_examples.each_with_index do |failure, index| | |
| @output.puts " #{index + 1}) #{failure['description']}" | |
| @output.puts " #{failure['location']}" if failure['location'] | |
| @output.puts " #{failure['message']}" if failure['message'] | |
| @output.puts " #{failure['formatted_backtrace']}" if failure['formatted_backtrace'] | |
| @output.puts | |
| # Print failed examples | |
| unless all_failed_examples.empty? | |
| @output.puts # Print a single blank line before "Failures:" | |
| @output.puts "Failures:" | |
| all_failed_examples.each_with_index do |failure, index| | |
| @output.puts " #{index + 1}) #{failure['description']}" | |
| @output.puts " #{failure['location']}" if failure['location'] | |
| @output.puts " #{failure['message']}" if failure['message'] | |
| @output.puts " #{failure['formatted_backtrace']}" if failure['formatted_backtrace'] |
| end | ||
| end | ||
|
|
||
| def render_consolidated_summary |
Copilot
AI
Jul 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method spans more than 10 lines, violating the custom guideline. Consider extracting the failure-printing loop and the summary-line formatting into separate private helper methods to keep each method under 10 lines.
| expected_processes.all? { |process| @process_summaries.key?(process) } | ||
| end | ||
|
|
||
| def wait_for_summaries |
Copilot
AI
Jul 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is longer than 10 lines. You could extract the timeout loop and the timeout-handling block into small, focused methods to comply with the method length constraint.
|
@copilot handle review comments, please. Before start, check the |
I've reviewed the Could you please clarify what specific review comments you'd like me to address? I can see that the code needs refactoring to comply with the coding standards (method length limits, class size limits, etc.), but I want to make sure I'm addressing the right concerns. |
…ing smaller classes Co-authored-by: vovka <127781+vovka@users.noreply.github.com>
This PR implements consolidated summary reporting across all parallel test processes, replacing the previous dummy summary output with comprehensive failure details and aggregated statistics.
Changes Made
Formatter Enhancements
example_failed()to collect detailed failure information including description, location, error message, and backtraceexample_pending()Orchestrator Enhancements
Example Output
Technical Details
summarymessage typeSummaryNotification,FailedExampleNotification)Testing
Fixes #26.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.