Skip to content

Conversation

sbryngelson
Copy link
Member

@sbryngelson sbryngelson commented Aug 10, 2025

User description

Adds progress counter to test.


PR Type

Enhancement


Description

  • Add progress counter to test execution output

  • Update job name formatting in CI script


Diagram Walkthrough

flowchart LR
  A["Test Execution"] --> B["Progress Counter"]
  B --> C["Display (n/total)"]
  D["CI Job"] --> E["Updated Job Name"]
Loading

File Walkthrough

Relevant files
Enhancement
test.py
Add progress tracking to test execution                                   

toolchain/mfc/test/test.py

  • Add global variables current_test_number and total_test_count for
    tracking progress
  • Initialize total_test_count with the number of test cases
  • Update test output header to include "Progress" column
  • Display progress counter "(n/total)" in test execution output
+9/-3     
Formatting
submit.sh
Fix job name formatting                                                                   

.github/workflows/frontier/submit.sh

  • Add space in SBATCH job name format from "MFC-" to "MFC-"
+1/-1     

@Copilot Copilot AI review requested due to automatic review settings August 10, 2025 13:24
@sbryngelson sbryngelson requested a review from a team as a code owner August 10, 2025 13:24
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

Progress counter may be incorrect when running cases concurrently because a shared global current_test_number is incremented without synchronization; output ordering and counts could interleave and misreport progress.

        if "inf," in output:
            raise MFCException(f"Test {case}: Post Process has detected an Infinity. You can find the run's output in {out_filepath}, and the case dictionary in {case.get_filepath()}.")

case.delete_output()

end_time = time.time()
duration = end_time - start_time

current_test_number += 1
progress_str = f"({current_test_number:3d}/{total_test_count:3d})"
cons.print(f"  {progress_str}    [bold magenta]{case.get_uuid()}[/bold magenta]    {duration:6.2f}    {case.trace}")
Global State

total_test_count and current_test_number are globals; ensure they are reset for successive invocations or when filtering/listing paths are used to avoid stale values.

current_test_number = 0
total_test_count = 0
errors = []

# pylint: disable=too-many-branches, trailing-whitespace
def __filter(cases_) -> typing.List[TestCase]:
    cases = cases_[:]

Copy link
Contributor

@Copilot 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 a progress counter to the MFC test runner, displaying the current test number and total test count during test execution. This enhancement improves user experience by providing visibility into test progress.

Key changes:

  • Added global variables to track current test number and total test count
  • Modified test output format to include progress information
  • Updated the test header to accommodate the new progress column

def _handle_case(case: TestCase, devices: typing.Set[int]):
# pylint: disable=global-statement, global-variable-not-assigned
global current_test_number
start_time = time.time()
Copy link
Preview

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

The global variable current_test_number is being modified in _handle_case which appears to be called from multiple threads based on the comment 'Run cases with multiple threads'. This creates a race condition where multiple threads could increment the counter simultaneously, leading to incorrect progress reporting.

Suggested change
start_time = time.time()
start_time = time.time()
# Ensure thread-safe increment of current_test_number
with current_test_number_lock:
current_test_number += 1

Copilot uses AI. Check for mistakes.

Comment on lines +276 to +277
current_test_number += 1
progress_str = f"({current_test_number:3d}/{total_test_count:3d})"
Copy link
Preview

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

This increment operation is not thread-safe. When multiple test cases run concurrently, the progress counter will be inaccurate due to race conditions. Consider using a thread-safe counter or moving this logic to a synchronized section.

Suggested change
current_test_number += 1
progress_str = f"({current_test_number:3d}/{total_test_count:3d})"
with current_test_number_lock:
current_test_number += 1
progress_str = f"({current_test_number:3d}/{total_test_count:3d})"

Copilot uses AI. Check for mistakes.

Copy link

qodo-merge-pro bot commented Aug 10, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Reset progress at test start

Also declare current_test_number as global in test() and reset it to 0 before
execution starts. Without resetting, previous runs may carry over the counter in
long-lived processes (e.g., interactive sessions).

toolchain/mfc/test/test.py [98-101]

 def test():
     # pylint: disable=global-statement, global-variable-not-assigned
-    global nFAIL, nPASS, nSKIP, total_test_count
+    global nFAIL, nPASS, nSKIP, total_test_count, current_test_number
     global errors
+    current_test_number = 0
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies and fixes a bug where current_test_number is not reset, which would cause incorrect progress reporting if test() is called multiple times.

Medium
Ensure fresh counter initialization

Initialize the progress counters immediately before starting execution to avoid
stale values across multiple runs or partial lists. This prevents incorrect
progress when reusing the same process or when tests are filtered.

toolchain/mfc/test/test.py [22-23]

+current_test_number = 0
+total_test_count = 0
 
-
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that counters should be reset for each test run, but its improved_code is identical to the existing_code and doesn't propose a concrete change.

Low
  • Update

@sbryngelson sbryngelson enabled auto-merge (squash) August 11, 2025 12:54
@sbryngelson sbryngelson disabled auto-merge August 11, 2025 12:54
@sbryngelson sbryngelson merged commit 4a00c56 into MFlowCode:master Aug 11, 2025
29 checks passed
@sbryngelson sbryngelson deleted the ci-improve branch August 14, 2025 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

1 participant