Skip to content

fix(schema): give Task.open_query a proper default and add schema tests#49

Open
wustwyh wants to merge 1 commit into
modelscope:mainfrom
wustwyh:fix/task-open-query-default-and-tests
Open

fix(schema): give Task.open_query a proper default and add schema tests#49
wustwyh wants to merge 1 commit into
modelscope:mainfrom
wustwyh:fix/task-open-query-default-and-tests

Conversation

@wustwyh

@wustwyh wustwyh commented Jun 24, 2026

Copy link
Copy Markdown

What changed\n\n- Replaced the FIXME comment on Task.open_query with an explicit default=False so callers can construct a Task without having to supply the new attribute.\n- Added ests/test_schema.py with unit tests for Task and TaskObjective covering defaults, overrides, optional fields, and serialization roundtrip.\n\n## Why\n\nThe Task schema currently required every instantiation site to provide open_query, even though the intent (per the FIXME) was for it to default to False. Giving it a proper default reduces boilerplate and avoids accidental breakage at call sites that haven't been updated yet. The new tests lock in the expected schema behavior and will catch future regressions.\n\n## Verification\n\n�ash\npython -m pytest tests/test_schema.py -v\n\n\nAll 10 tests pass.

- Replace the FIXME comment on Task.open_query with an explicit default=False so callers can construct a Task without specifying it.

- Add unit tests for Task and TaskObjective covering defaults, overrides, optional fields, and serialization roundtrip.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request sets a default value of False for the open_query field in the Task schema and introduces a comprehensive suite of unit tests in tests/test_schema.py to verify the behavior of Task and TaskObjective. The review feedback recommends removing dynamic sys.path modifications and unused imports from the test file, replacing the Pydantic v2 model_dump() call with the Pydantic v1 .dict() method to ensure backward compatibility, and removing the outdated if __name__ == '__main__': pytest.main(...) boilerplate.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread tests/test_schema.py
Comment on lines +1 to +8
import sys
from pathlib import Path

import pytest

ROOT_DIR = Path(__file__).resolve().parents[1]
if str(ROOT_DIR) not in sys.path:
sys.path.insert(0, str(ROOT_DIR))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Modifying sys.path dynamically within test files is an anti-pattern that can mask import issues and make tests behave inconsistently. Since the recommended way to run tests is via python -m pytest (as mentioned in the PR description), the project root is automatically added to sys.path by Python. Therefore, this dynamic path insertion and the unused sys and pathlib imports can be safely removed.

Suggested change
import sys
from pathlib import Path
import pytest
ROOT_DIR = Path(__file__).resolve().parents[1]
if str(ROOT_DIR) not in sys.path:
sys.path.insert(0, str(ROOT_DIR))
import pytest

Comment thread tests/test_schema.py
Comment on lines +90 to +91
data = objective.model_dump()
restored = TaskObjective(**data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The rest of the codebase (e.g., task_manager.py) consistently uses Pydantic v1 APIs such as .dict(), .json(), parse_obj(), and parse_raw(). Using model_dump() (a Pydantic v2 API) here will cause an AttributeError if the environment is running Pydantic v1. To maintain compatibility with the rest of the codebase, use .dict() instead.

Suggested change
data = objective.model_dump()
restored = TaskObjective(**data)
data = objective.dict()
restored = TaskObjective(**data)

Comment thread tests/test_schema.py
Comment on lines +100 to +101
if __name__ == "__main__":
pytest.main([__file__, "-v"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Running tests by executing the test file directly via if __name__ == '__main__': pytest.main(...) is an outdated practice. Modern test suites rely entirely on the pytest CLI runner. Removing this boilerplate keeps the test files clean and standard.

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.

1 participant