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

[bazel] Make the atoms tests actually run #15510

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented Mar 25, 2025

PR Type

Bug fix, Tests


Description

  • Fixed the Bazel closure_test_suite name to ensure proper execution.

  • Included all relevant files in the test data using glob.


Changes walkthrough 📝

Relevant files
Tests
BUILD.bazel
Fixed test suite name and expanded test data                         

javascript/atoms/BUILD.bazel

  • Renamed closure_test_suite from test to tests.
  • Added glob(["**/*"]) to include all files in test data.
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Narrow glob pattern scope

    Using glob(["/*"]) is overly broad and can include unnecessary files,
    potentially causing build inefficiency. Consider using a more specific glob
    pattern that targets only test files or resources needed for tests.**

    javascript/atoms/BUILD.bazel [399-402]

     data = [
         ":atoms",
         ":deps",
    -] + glob(["**/*"]),
    +] + glob(["test/**/*.js", "test/**/*.html"]),
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that using glob(["**/*"]) is too broad and could include unnecessary files in the test data, potentially causing build inefficiency. The proposed more specific glob pattern targeting only test JS and HTML files is a meaningful improvement for build performance and clarity.

    Medium
    • More

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant