Skip to content

test(python): time template protocol initial delay from connect - #4155

Merged
jmthomas merged 1 commit into
mainfrom
fix/template-protocol-initial-delay-test
Oct 27, 2026
Merged

jmthomas merged 1 commit into
mainfrom
fix/template-protocol-initial-delay-test

Conversation

@ryanmelt

@ryanmelt ryanmelt commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

What changed

  • test_template_protocol.py: test_waits_before_writing_during_the_initial_delay_period now starts its timer before interface.connect(). It used to start the timer after connect() returned.

Why it changed

TemplateProtocol sets its initial-delay deadline in connect_reset(), which runs during connect(). Because the test started timing after connect() returned, it missed however long connect() took. On a slow runner the measured wait could come in just under the configured delay. That happened in unit-test (3.13) on #3948: AssertionError: 0.019564390182495117 not greater than or equal to 0.02. The protocol was working correctly. Only the measurement was wrong.

Testing strategy

  • All 22 tests in test_template_protocol.py pass locally.
  • The test still checks only the minimum wait. test_waits_no_longer_than_the_initial_delay_before_writing already checks the protocol doesn't wait longer than configured.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Updated timing coverage for protocol connection delays to include the time spent connecting before the first write.

The initial read delay is measured from connect, but the test started its
timer after connect returned, so the measured wait undercounted by connect's
own duration and failed on a slow runner (0.0196s < 0.02s).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 589aff7c-3bb9-42c6-9a2d-dc7a896d3b29
Commits

Reviewing files that changed from the base of the PR and between d2ed704 and b459363.

Files selected for processing: 1
  • openc3/python/test/interfaces/protocols/test_template_protocol.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

The test starts measuring elapsed time before connecting and checks the elapsed time after writing.

Changes: Template protocol test

Layer / File(s) Summary
Elapsed-time measurement
openc3/python/test/interfaces/protocols/test_template_protocol.py
The test measures elapsed time from before connect() through write().

Priority: Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Suggested labels: python, testing

Suggested reviewers: emilyragan

Merge Risk: Minimal · up to b4593

No actionable merge risk remains in this test-only change.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: measuring the TemplateProtocol initial delay from the connection step.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.24%. Comparing base (d2ed704) to head (b459363).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4155      +/-   ##
==========================================
- Coverage   80.28%   80.24%   -0.05%     
==========================================
  Files         909      909              
  Lines       70551    70551              
  Branches     2741     2741              
==========================================
- Hits        56643    56613      -30     
- Misses      13237    13268      +31     
+ Partials      671      670       -1     
Flag Coverage Δ
frontend 68.44% <ø> (-0.11%) ⬇️
python 78.71% <ø> (-0.05%) ⬇️
ruby-api 83.32% <ø> (-0.12%) ⬇️
ruby-backend 86.42% <ø> (ø)

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

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jmthomas
jmthomas merged commit 5b3b859 into main Oct 27, 2026
50 of 57 checks passed
@jmthomas
jmthomas deleted the fix/template-protocol-initial-delay-test branch October 27, 2026 03:46
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.

3 participants