Skip to content

Conversation

@d-chambers
Copy link
Contributor

@d-chambers d-chambers commented Sep 19, 2025

Description

This PR improves DASCore's test coverage slightly.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings or appropriate doc page.
  • included a test. See testing guidelines.
  • your name has been added to the contributors page (docs/contributors.md).
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

Summary by CodeRabbit

  • Tests
    • Expanded coverage for updating with specific paths (relative and absolute).
    • Added unit tests for TDMS utilities, including handling of unsupported data types and timestamp parsing for valid and invalid inputs.
    • Verified clear error messaging for unsupported Terra15 file versions.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Adds new and expanded tests: a DirectoryIndexer.update paths test, a TDMS utilities test module for error handling and timestamp parsing, and a Terra15 unsupported version error test. No production code changes.

Changes

Cohort / File(s) Summary of changes
Indexer update path handling tests
tests/test_io/test_indexer.py
Adds TestUpdate.test_update_with_specific_paths verifying DirectoryIndexer.update accepts paths and handles relative and absolute inputs, returning updated contents with entries. Tests only.
TDMS utils tests
tests/test_io/test_tdms/test_tdms_utils.py
New test module: asserts type_not_supported raises NotImplementedError with expected message; validates parse_time_stamp returns None for invalid inputs and a datetime for a constructed LabVIEW-era timestamp.
Terra15 unsupported version test
tests/test_io/test_terra15/test_terra15.py
Adds TestTerra15.test_unsupported_version_error asserting _get_version_data_node raises NotImplementedError on unknown file_version; includes ClassVar import for mock typing.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive The PR description includes a brief summary and the checklist from the repository template but is overly terse and lacks details about which files/tests were added, the motivation for the changes, and any related issues or documentation references, making it difficult to assess scope and impact. Please expand the Description to list the specific test files and behaviors added or modified, explain the motivation or coverage gaps addressed, link any related issues or docs, and update the checklist and "ready_for_review" tag when the PR is ready.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "improve coverage" is short and correctly signals the PR's intent to increase test coverage (the changes add tests for the indexer, TDMS utilities, and Terra15), but it is generic and does not specify which areas or tests were changed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch coverage

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb657eb and bf93f0e.

📒 Files selected for processing (3)
  • tests/test_io/test_indexer.py (1 hunks)
  • tests/test_io/test_tdms/test_tdms_utils.py (1 hunks)
  • tests/test_io/test_terra15/test_terra15.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_io/test_tdms/test_tdms_utils.py (1)
dascore/io/tdms/utils.py (2)
  • parse_time_stamp (87-99)
  • type_not_supported (19-21)
tests/test_io/test_indexer.py (1)
dascore/io/indexer.py (2)
  • update (90-95)
  • update (309-346)
tests/test_io/test_terra15/test_terra15.py (1)
dascore/io/terra15/utils.py (1)
  • _get_version_data_node (69-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: test_code_min_deps (macos-latest, 3.12)
  • GitHub Check: test_code_min_deps (windows-latest, 3.12)
  • GitHub Check: test_code_min_deps (windows-latest, 3.13)
  • GitHub Check: test_code_min_deps (macos-latest, 3.13)
  • GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
  • GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
🔇 Additional comments (6)
tests/test_io/test_indexer.py (1)

279-297: LGTM! Good test for paths parameter coverage.

This test effectively covers the paths parameter functionality of the DirectoryIndexer.update() method, testing both relative and absolute path scenarios. The test correctly verifies that the indexer can handle specific path filtering and returns appropriate results.

tests/test_io/test_terra15/test_terra15.py (2)

6-6: Appropriate import addition.

The ClassVar import is correctly added to support the mock object's type annotation in the new test method.


75-87: Excellent test for error handling coverage.

This test effectively covers the error path in _get_version_data_node function for unsupported Terra15 versions. The mock approach is clean and appropriate for testing this specific error condition.

tests/test_io/test_tdms/test_tdms_utils.py (3)

13-18: Good coverage for error function.

This test correctly verifies that type_not_supported raises the expected NotImplementedError with the proper message, providing complete coverage for this utility function.


20-34: Comprehensive testing of edge cases.

This test method thoroughly covers the various invalid input scenarios for parse_time_stamp, including zero values and None inputs. The test cases align well with the function's logic that returns None when fractions + seconds <= 0 or when either parameter is None.


36-48: Effective test for valid timestamp parsing.

This test verifies the positive path of parse_time_stamp with a realistic timestamp calculation (100 years after the 1904 LabVIEW epoch). The assertions appropriately check both the return type and that the resulting year is valid.

Comment on lines +42 to +44
# Use a timestamp that represents a valid date after 1904
seconds = 365 * 24 * 3600 * 100 # 100 years after 1904
fractions = 0.5 # Some fractional seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

🧩 Analysis chain

Verify the timestamp calculation accuracy.

The test uses 100 years after 1904 which should result in a 2004 date. LabVIEW's timestamp epoch starts at January 1, 1904 00:00:00.00 UTC, so the calculation 365 * 24 * 3600 * 100 (100 years of seconds) is reasonable for testing purposes.

Let me verify the exact calculation is correct:


🏁 Script executed:

#!/bin/bash
# Description: Verify the timestamp calculation for 100 years after 1904

# Calculate seconds in 100 years (accounting for leap years)
python3 << 'EOF'
import datetime

# Calculate the exact number of seconds between 1904-01-01 and 2004-01-01
start_date = datetime.datetime(1904, 1, 1)
end_date = datetime.datetime(2004, 1, 1)
diff = end_date - start_date
seconds_in_100_years = int(diff.total_seconds())

print(f"Exact seconds in 100 years (1904-2004): {seconds_in_100_years}")
print(f"Test value used: {365 * 24 * 3600 * 100}")
print(f"Difference: {seconds_in_100_years - (365 * 24 * 3600 * 100)}")

# Test the calculation with the exact value
test_seconds = 365 * 24 * 3600 * 100  # 100 years after 1904
test_fractions = 0.5
result_date = datetime.timedelta(0, test_fractions * 2**-64 + test_seconds) + datetime.datetime(1904, 1, 1)
print(f"Resulting date from test: {result_date}")
print(f"Year: {result_date.year}")
EOF

Length of output: 155


Fix timestamp calculation: account for leap years (use exact seconds or datetime arithmetic).

365 * 24 * 3600 * 100 = 3,153,600,000 but the exact seconds 1904‑01‑01 → 2004‑01‑01 = 3,155,760,000 (diff 2,160,000s = 25 days). With the current value the test produces 2003‑12‑07 instead of 2004‑01‑01. Replace the constant with 3,155,760,000 or compute the timestamp from datetime(1904,1,1) + timedelta(...) to avoid the off-by-leap-year error.

tests/test_io/test_tdms/test_tdms_utils.py:42-44

@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.92%. Comparing base (49c7503) to head (9ef69dc).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   99.88%   99.92%   +0.03%     
==========================================
  Files         124      124              
  Lines       10336    10336              
==========================================
+ Hits        10324    10328       +4     
+ Misses         12        8       -4     
Flag Coverage Δ
unittests 99.92% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-chambers d-chambers merged commit 61ea3c7 into master Sep 19, 2025
22 checks passed
@d-chambers d-chambers deleted the coverage branch September 19, 2025 11:35
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.

2 participants