Skip to content

fix: replace manual nanosecond calculations with time.Hour in frequency tests#733

Open
krishnavermaKV wants to merge 2 commits intoOneBusAway:mainfrom
krishnavermaKV:fix/frequency-time-clean
Open

fix: replace manual nanosecond calculations with time.Hour in frequency tests#733
krishnavermaKV wants to merge 2 commits intoOneBusAway:mainfrom
krishnavermaKV:fix/frequency-time-clean

Conversation

@krishnavermaKV
Copy link
Copy Markdown

🚀 Overview

This PR improves time handling in frequency-related tests by replacing manual nanosecond calculations (e.g., * 1e9) with Go’s time package.

🔧 Changes

  • Replaced usages of * 1e9 with time.Hour / time.Second
  • Improved readability and type safety of time values in tests

🎯 Why This Matters

  • Avoids implicit float → int conversions
  • Prevents potential precision issues
  • Aligns with idiomatic Go practices (time.Duration)
  • Improves code clarity and maintainability

🧪 Testing

  • Ran existing tests to ensure no regressions
  • All frequency-related test cases behave as expected

🔗 Related Issue

Fixes #727

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 17, 2026

CLA assistant check
All committers have signed the CLA.

@krishnavermaKV
Copy link
Copy Markdown
Author

krishnavermaKV commented Mar 18, 2026

@aaronbrethorst can you please review the pr

@krishnavermaKV
Copy link
Copy Markdown
Author

krishnavermaKV commented Mar 19, 2026

@CLAassistant can you run workflows

krishnavermaKV

This comment was marked as off-topic.

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

Hey Krishna, this is a nice cleanup — replacing 3600 * 1e9 with time.Hour makes these tests much more readable at a glance. The change is mechanically correct and all frequency tests pass.

Important Issues (1 found)

1. go fmt not run — formatting errors in the file

The project requires running go fmt ./... before committing (see CLAUDE.md). There are two formatting issues:

gtfsdb/bulk_insert_frequency_test.go:7 — The "time" import uses spaces instead of a tab:

// Current (spaces):
    "time"
// Should be (tab):
	"time"

gtfsdb/bulk_insert_frequency_test.go:107 — Comment alignment has an extra space after the change shortened the line:

// Current:
HeadwaySecs: 600,                   // 10 minutes
// After go fmt:
HeadwaySecs: 600,                  // 10 minutes

Running go fmt ./gtfsdb/bulk_insert_frequency_test.go will fix both automatically.

Strengths

  • Clean, focused change — one file, one concern
  • int64(6 * time.Hour) is immediately readable vs int64(6 * 3600 * 1e9)
  • The loop case on line 131 (time.Duration(i) * time.Hour) correctly handles the type conversion

Recommended Action

  1. Run go fmt ./... and commit the result
  2. Please go read this: https://opentransitsoftwarefoundation.org/2025/12/our-policy-on-ai-generated-contributions/

@krishnavermaKV
Copy link
Copy Markdown
Author

krishnavermaKV commented Mar 26, 2026

Thanks for the review @aaronbrethorst
I've run "go fmt ./..." and fixed the formatting issues in the test file.
Regarding the AI policy: I used AI tools to assist me and for learning new things and technology's, and I verified all changes manually and ensured they follow the project's conventions and correctness.
Please let me know if anything else needs adjustment.

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.

Inconsistent time handling and non-deterministic ordering in bulkInsertFrequencies tests

3 participants