To support for importing historical data #1008
Conversation
|
""" WalkthroughThe changes add an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant DateTimeParser
participant API
User->>CLI: run build/tests/session --timestamp <datetime>
CLI->>DateTimeParser: Parse --timestamp value
DateTimeParser-->>CLI: datetime object (with tzinfo)
CLI->>API: Send payload including timestamp (ISO 8601)
API-->>CLI: Response
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/utils/test_click.py (1)
51-75:⚠️ Potential issueFix type annotation and improve test structure.
The test class has a critical type annotation error that's causing the mypy failure. The
actualvariable is declared asstrbut assigned adatetime.datetimeobject.Apply this diff to fix the type annotation:
- def test_conversion(self): - def scenario(expected: str, *args): - actual: str = "" + def test_conversion(self): + def scenario(expected: datetime.datetime, *args): + actual: datetime.datetime = NoneAlso, consider adding a test case for error handling:
+ # Test invalid datetime format + with self.assertRaises(click.BadParameter): + CliRunner().invoke(time, ['-t', 'invalid-datetime'])Optional improvement: Add more comprehensive test coverage.
Consider testing edge cases like:
- Empty timestamp argument
- Different timezone formats
- Invalid timezone names
def test_invalid_formats(self): @click.command() @click.option('-t', 'timestamp', type=DATETIME_WITH_TZ) def time(timestamp): pass runner = CliRunner() # Test invalid format result = runner.invoke(time, ['-t', 'not-a-date']) self.assertNotEqual(0, result.exit_code) # Test invalid timezone result = runner.invoke(time, ['-t', '2023-10-01T12:00:00+25:00']) self.assertNotEqual(0, result.exit_code)🧰 Tools
🪛 GitHub Actions: Python package
[error] 65-65: mypy: Incompatible types in assignment (expression has type "datetime", variable has type "str") (assignment)
🧹 Nitpick comments (4)
launchable/utils/click.py (1)
99-111: Improve exception handling and error message clarity.The implementation is solid, but consider these improvements:
dateutil.parser.parse()can raise other exceptions besidesValueError(e.g.,TypeError,OverflowError)- The error message could be more descriptive about acceptable formats
def convert(self, value: str, param: Optional[click.core.Parameter], ctx: Optional[click.core.Context]): try: dt = dateutil.parser.parse(value) if dt.tzinfo is None: return dt.replace(tzinfo=tzlocal()) return dt - except ValueError: - self.fail("Expected datetime like 2023-10-01T12:00:00 but got '{}'".format(value), param, ctx) + except (ValueError, TypeError, OverflowError) as e: + self.fail("Expected datetime in ISO format (YYYY-MM-DDTHH:MM:SS[±HH:MM]) or similar, but got '{}'. Error: {}".format(value, str(e)), param, ctx)launchable/commands/record/build.py (1)
100-106: Consider clarifying the TZD abbreviation in help text.The CLI option implementation is excellent with comprehensive help text. Minor suggestion: "TZD" might not be familiar to all users.
- help='Used to overwrite the build time when importing historical data. Note: Format must be `YYYY-MM-DDThh:mm:ssTZD` or `YYYY-MM-DDThh:mm:ss` (local timezone applied)', # noqa: E501 + help='Used to overwrite the build time when importing historical data. Note: Format must be `YYYY-MM-DDThh:mm:ss±HH:MM` (with timezone) or `YYYY-MM-DDThh:mm:ss` (local timezone applied)', # noqa: E501launchable/commands/record/tests.py (1)
415-421: Consider simplifying the timestamp check condition for readability.The logic is correct but could be more readable. The condition properly bypasses the timestamp check when
--timestampis provided.- if ( - not self.is_allow_test_before_build # nlqa: W503 - and not self.is_no_build # noqa: W503 - and timestamp is None # noqa: W503 - and self.check_timestamp # noqa: W503 - and ctime.timestamp() < record_start_at.timestamp() # noqa: W503 - ): + should_skip_old_report = ( + not self.is_allow_test_before_build + and not self.is_no_build + and timestamp is None + and self.check_timestamp + and ctime.timestamp() < record_start_at.timestamp() + ) + if should_skip_old_report:tests/commands/record/test_build.py (1)
270-270: Fix misplaced comment.The comment appears to be copied from another test method and doesn't match the new
test_with_timestampfunctionality.-# make sure the output of git-submodule is properly parsed +# verify that --timestamp option correctly adds timestamp to payload
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
launchable/commands/record/build.py(4 hunks)launchable/commands/record/tests.py(5 hunks)launchable/utils/click.py(2 hunks)tests/commands/record/test_build.py(7 hunks)tests/utils/test_click.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/commands/record/test_build.py (2)
tests/cli_test_case.py (5)
CliTestCase(21-341)cli(200-211)assert_success(213-214)payload(278-289)assert_json_orderless_equal(297-313)launchable/utils/session.py (1)
read_build(19-30)
🪛 GitHub Actions: Python package
tests/utils/test_click.py
[error] 5-5: mypy: Cannot find implementation or library stub for module named "zoneinfo" (import-not-found)
[error] 65-65: mypy: Incompatible types in assignment (expression has type "datetime", variable has type "str") (assignment)
🔇 Additional comments (15)
launchable/utils/click.py (2)
6-8: LGTM: Clean import additions for datetime parsing.The imports for
dateutil.parserandtzlocalare appropriate for the new datetime parsing functionality.
117-117: LGTM: Consistent constant naming pattern.The
DATETIME_WITH_TZconstant follows the established naming convention used by other parameter types in this file.launchable/commands/record/build.py (4)
1-1: LGTM: Necessary import for timestamp functionality.The
datetimeimport is required for the new timestamp parameter type annotation.
15-15: LGTM: Correct import for the new parameter type.The
DATETIME_WITH_TZimport is appropriately added alongside the existingKEY_VALUEimport.
113-113: LGTM: Function signature correctly updated.The
timestampparameter is properly typed and positioned at the end of the parameter list.
337-338: LGTM: Proper timestamp handling in payload.The implementation correctly formats the timestamp as ISO string when provided and sets it to
Noneotherwise. The conditional expression is clean and appropriate.launchable/commands/record/tests.py (4)
19-19: LGTM: Correct import for datetime parameter type.The
DATETIME_WITH_TZimport is appropriately added to support the new timestamp functionality.
162-168: LGTM: Comprehensive CLI option with clear help text.The
--timestampoption is well-documented and follows the same pattern as the build command. The help text clearly explains the use case for importing historical data.
187-187: LGTM: Function signature properly updated.The
timestampparameter is correctly typed and positioned appropriately in the function signature.
453-456: LGTM: Correct timestamp override for historical data.The implementation properly overrides the
createdAtfield with the provided timestamp when importing historical data. The ISO format conversion is appropriate.tests/commands/record/test_build.py (2)
62-64: LGTM: Consistent test payload updates.All existing test cases have been properly updated to include the new
"timestamp": Nonefield in their payload assertions. This ensures backward compatibility testing.Also applies to: 97-99, 129-131, 188-190, 218-220, 258-260
271-297: LGTM: Comprehensive test for timestamp functionality.The test properly verifies that:
- The
--timestampoption is accepted- The timestamp is correctly formatted as ISO string in the payload
- The build name is recorded correctly
- Other payload fields remain consistent
The test follows established patterns and uses appropriate mocking.
tests/utils/test_click.py (3)
1-2: LGTM: Standard datetime imports added.The datetime and timezone imports are correctly added to support the new timestamp functionality.
9-9: LGTM: Required dateutil import added.The
tzlocalimport is correctly added to support local timezone handling in the tests.
11-11: LGTM: New parameter type import added.The
DATETIME_WITH_TZimport is correctly added to test the new datetime parameter type functionality.
Konboi
force-pushed
the
timestamp-option
branch
2 times, most recently
from
5326e95 to
29ad71f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/utils/test_click.py (1)
5-5:⚠️ Potential issueRemove unused zoneinfo import that causes compatibility issues.
The
zoneinfoimport is causing Python version compatibility issues (only available in Python 3.9+) and is not actually used anywhere in the code, as confirmed by static analysis.Apply this diff to remove the unused import:
-from zoneinfo import ZoneInfo🧰 Tools
🪛 Ruff (0.11.9)
5-5:
zoneinfo.ZoneInfoimported but unusedRemove unused import:
zoneinfo.ZoneInfo(F401)
🪛 GitHub Actions: Python package
[error] 5-5: mypy error: Cannot find implementation or library stub for module named "zoneinfo" (import-not-found). See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
🧹 Nitpick comments (1)
tests/utils/test_click.py (1)
51-74: Enhance test coverage for the new timestamp feature.The basic test implementation is correct and follows good patterns. However, consider adding edge case testing to ensure robustness of the new timestamp CLI option.
Consider adding these additional test scenarios for better coverage:
+ # Test edge cases + with self.assertRaises(click.BadParameter): + CliRunner().invoke(time, ['-t', 'invalid-date']) + + with self.assertRaises(click.BadParameter): + CliRunner().invoke(time, ['-t', '2023-13-01 12:00:00']) # Invalid month + + # Test additional timezone formats + scenario(datetime.datetime(2023, 10, 1, 15, 30, 0, tzinfo=timezone(timedelta(hours=5, minutes=30))), + '-t', '2023-10-01T15:30:00+05:30') # IST timezone
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
launchable/commands/record/build.py(4 hunks)launchable/commands/record/tests.py(5 hunks)launchable/utils/click.py(2 hunks)tests/utils/test_click.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- launchable/commands/record/tests.py
- launchable/utils/click.py
- launchable/commands/record/build.py
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/utils/test_click.py
5-5: zoneinfo.ZoneInfo imported but unused
Remove unused import: zoneinfo.ZoneInfo
(F401)
🪛 GitHub Actions: Python package
tests/utils/test_click.py
[error] 5-5: mypy error: Cannot find implementation or library stub for module named "zoneinfo" (import-not-found). See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/utils/test_click.py (1)
5-5:⚠️ Potential issueFix Python version compatibility issue with zoneinfo.
The
zoneinfomodule is only available in Python 3.9+, which could cause compatibility issues with older Python versions. This import is also unused in the current code.Remove the unused import and use
dateutil.tz.gettz()if timezone handling is needed:-from zoneinfo import ZoneInfo🧰 Tools
🪛 Ruff (0.11.9)
5-5:
zoneinfo.ZoneInfoimported but unusedRemove unused import:
zoneinfo.ZoneInfo(F401)
🧹 Nitpick comments (1)
tests/utils/test_click.py (1)
51-74: Test implementation looks good with minor naming improvement.The test class correctly validates the
DATETIME_WITH_TZClick parameter type with comprehensive scenarios covering naive datetime strings, UTC offsets, and Z suffix notation. The test logic properly verifies timezone-aware datetime conversion.Consider renaming the
expectedparameter in the scenario function for clarity:- def scenario(expected: str, *args): + def scenario(expected_datetime: datetime.datetime, *args):And update the parameter type annotation since it expects a datetime object, not a string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
launchable/commands/record/build.py(4 hunks)launchable/commands/record/tests.py(5 hunks)launchable/utils/click.py(2 hunks)tests/utils/test_click.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- launchable/utils/click.py
- launchable/commands/record/tests.py
- launchable/commands/record/build.py
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/utils/test_click.py
5-5: zoneinfo.ZoneInfo imported but unused
Remove unused import: zoneinfo.ZoneInfo
(F401)
🪛 GitHub Actions: Python package
tests/utils/test_click.py
[error] 1-1: Test failure detected during test run.
This comment has been minimized.
This comment has been minimized.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
launchable/commands/helper.py (1)
96-107:⚠️ Potential issueFix inconsistent timestamp handling between execution paths.
The timestamp parameter is only passed to
session_commandin the normal execution path (line 139) but not in the no-build execution path (lines 96-107). This inconsistency could lead to different behavior when using the--timestampoption with--no-build.Apply this diff to ensure consistent timestamp handling:
context.invoke( session_command, build_name=NO_BUILD_BUILD_NAME, save_session_file=True, print_session=False, flavor=flavor, is_observation=is_observation, links=links, is_no_build=is_no_build, lineage=lineage, test_suite=test_suite, + timestamp=timestamp, )Also applies to: 128-140
🧹 Nitpick comments (1)
launchable/commands/helper.py (1)
68-68: Add documentation for the timestamp parameter.The new
timestampparameter should be documented in the function's docstring Args section for consistency with other parameters.lineage: lineage option value test_suite: --test-suite option value + timestamp: Optional datetime for session creation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
launchable/commands/helper.py(3 hunks)launchable/commands/record/tests.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- launchable/commands/record/tests.py
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (ubuntu-22.04, 3.12)
- GitHub Check: build (ubuntu-22.04, 3.11)
- GitHub Check: build (ubuntu-22.04, 3.10)
- GitHub Check: build (ubuntu-22.04, 3.9)
- GitHub Check: build (ubuntu-22.04, 3.8)
- GitHub Check: build (ubuntu-22.04, 3.7)
🔇 Additional comments (2)
launchable/commands/helper.py (2)
1-1: LGTM: Import statement is correctly placed.The datetime import is appropriately added to support the new timestamp functionality.
139-139: LGTM: Timestamp parameter correctly passed to session_command.The timestamp parameter is properly passed to the session_command in the normal execution path, enabling historical data import functionality.
To use PTS, data is required, but collecting new data from scratch takes time.
Therefore, we will provide a
--timestampoption to allow importing past data as well.Summary by CodeRabbit