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

Cover more command strings in run_command.sh #4114

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

jacobhinkle
Copy link
Collaborator

This fixes a bug in codediff CI where we currently do not recognize some tests as GOOGLETEST because they are launched through a script that shards the tests. This adds a substring to match that case and adds a couple other cases.

This fixes a bug in codediff CI where we currently do not recognize some
tests as GOOGLETEST because they are launched through a script that
shards the tests. This adds a substring to match that case and adds a
couple other cases.
@jacobhinkle jacobhinkle requested a review from naoyam March 20, 2025 13:16
@jacobhinkle
Copy link
Collaborator Author

!test --diff

Copy link

Description

  • Add bash version check for fallthrough token

  • Extend command string matching for GOOGLETEST

  • Extend command string matching for GOOGLEBENCH

  • Extend command string matching for PYTEST


Changes walkthrough 📝

Relevant files
Enhancement
run_command.sh
Enhance command string matching and add bash version check

tools/codediff/run_command.sh

  • Added bash version check for fallthrough token
  • Extended command string matching for GOOGLETEST, GOOGLEBENCH, and
    PYTEST
  • +17/-2   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Bash Version Check

    The script checks for bash version 4.0+ and exits if the version is lower. This might be restrictive and could prevent the script from running on systems with older bash versions. Consider whether this requirement is necessary or if there are alternatives.

    if [ "${BASH_VERSINFO:-0}" -lt 4 ]
    then
        # Required for fallthrough token ;& in case statements
        echo "This script requires bash 4.0+"
        exit 1
    fi
    Fallthrough Token Usage

    The ;& token is used for fallthrough in case statements, which is a bash 4.0+ feature. Ensure that this usage is intentional and that it does not introduce unexpected behavior. Also, verify that all cases where fallthrough is used are necessary.

        ;&
    *tutorial_*)
        ;&
    *split_binary_nvfuser_tests*)
    Command Type Matching

    The script now includes additional patterns for matching command types. Ensure that these new patterns do not inadvertently match unintended commands and that they accurately reflect the intended command types.

    *tutorial_*)
        ;&
    *split_binary_nvfuser_tests*)
        commandtype="GOOGLETEST"
        ;;
    *split_nvbench*)
        ;&
    *nvfuser_bench*)
        commandtype="GOOGLEBENCH"
        ;;
    *pytest*)
        ;&
    *tests/python*)
        ;&
    *python_tests*)

    Copy link
    Collaborator

    @naoyam naoyam left a comment

    Choose a reason for hiding this comment

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

    Thanks for the fix!

    @naoyam naoyam merged commit b8f9198 into main Mar 20, 2025
    54 of 56 checks passed
    @naoyam naoyam deleted the jh/update_codediff_runcommand branch March 20, 2025 15:04
    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.

    None yet

    2 participants