Skip to content

fix old assert#621

Merged
MaojiaSheng merged 1 commit intomainfrom
fix/skill_tool_test_bug
Mar 15, 2026
Merged

fix old assert#621
MaojiaSheng merged 1 commit intomainfrom
fix/skill_tool_test_bug

Conversation

@BytedanceFu
Copy link
Copy Markdown
Collaborator

@BytedanceFu BytedanceFu commented Mar 15, 2026

Description

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • Some test scripts became outdated due to code revisions, so they are being updated.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

@BytedanceFu BytedanceFu requested a review from MaojiaSheng March 15, 2026 04:29
@BytedanceFu BytedanceFu self-assigned this Mar 15, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 15, 2026

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

@BytedanceFu BytedanceFu removed their assignment Mar 15, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 15, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix contradictory test assertion

The test name test_suffix_like_weather_usage_does_not_match_weather indicates the
candidate should NOT match "weather", but the new assertion expects skill_name to be
"weather". This creates a contradiction between the test's purpose and its
assertion. Verify the intended behavior: either update the test name or revert the
assertion.

tests/unit/tool_skill/test_tool_skill_calibration.py [71]

-assert (tool_name, skill_name, status) == ("", "weather", "completed")
+assert (tool_name, skill_name, status) == ("", "", "completed")
Suggestion importance[1-10]: 8

__

Why: The test name test_suffix_like_weather_usage_does_not_match_weather indicates the candidate should NOT match "weather", but the PR's assertion expects skill_name to be "weather", creating a contradiction. This is a critical test correctness issue.

Medium

@MaojiaSheng MaojiaSheng merged commit 48fc7e2 into main Mar 15, 2026
6 checks passed
@MaojiaSheng MaojiaSheng deleted the fix/skill_tool_test_bug branch March 15, 2026 05:38
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaojiaSheng MaojiaSheng MaojiaSheng approved these changes

Assignees

No one assigned

Labels

Review effort 1/5

Projects

OpenViking project
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BytedanceFu @MaojiaSheng