test: Add unit tests for untested stdlib::time native functions - #434
Conversation
Added unit tests to `src/stdlib/time.rs` to verify the functionality of previously untested native functions including `native_today`, `native_create_date`, `native_add_days`, and `native_days_between`. These tests cover positive validation paths and explicit failure modes (such as invalid month or day bounds in `create_date`, or adding negative days in `add_days`). Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughAdded unit tests for date-related stdlib natives: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6678a485c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let expected = Local::now().date_naive(); | ||
| assert_eq!(*d, expected); |
There was a problem hiding this comment.
native_today test against midnight rollover
This assertion can fail nondeterministically if the test executes across a local-date boundary: native_today(vec![]) captures one Local::now() instant, then expected is computed from a later Local::now(). Around midnight, those dates can differ by one day even when native_today is correct, causing flaky CI runs. Capture the expected date window around the call (or compare within an allowed one-day boundary) to make the test deterministic.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds unit tests for previously untested stdlib::time native functions to increase coverage and guard against regressions in date-related operations.
Changes:
- Add unit tests for
native_today,native_create_date,native_add_days, andnative_days_between. - Validate basic success and some error-path behavior for date creation and arithmetic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let result = native_today(vec![]); | ||
| assert!(result.is_ok()); | ||
| if let Value::Date(d) = result.unwrap() { | ||
| let expected = Local::now().date_naive(); | ||
| assert_eq!(*d, expected); |
There was a problem hiding this comment.
test_native_today can be flaky around midnight because native_today() and Local::now().date_naive() are called at different times; if the date rolls over between those calls the assertion can fail intermittently in CI. Consider capturing the date both immediately before and after calling native_today() and asserting the returned date matches either value (or otherwise structuring the test so it doesn’t depend on wall-clock timing).
| let result = native_today(vec![]); | |
| assert!(result.is_ok()); | |
| if let Value::Date(d) = result.unwrap() { | |
| let expected = Local::now().date_naive(); | |
| assert_eq!(*d, expected); | |
| let before = Local::now().date_naive(); | |
| let result = native_today(vec![]); | |
| assert!(result.is_ok()); | |
| if let Value::Date(d) = result.unwrap() { | |
| let after = Local::now().date_naive(); | |
| assert!(*d == before || *d == after, "native_today returned unexpected date: {}", d); |
| Value::Number(32.0), // Invalid day | ||
| ]; | ||
| let result = native_create_date(args); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
test_native_create_date_invalid_day only exercises the explicit day range check (32), but doesn’t cover the NaiveDate::from_ymd_opt(...) == None branch for dates that are in-range yet invalid (e.g., 2023-02-30). Adding a test for an in-range-but-impossible date would cover that failure path and better validate native_create_date’s behavior.
| Value::Number(32.0), // Invalid day | |
| ]; | |
| let result = native_create_date(args); | |
| assert!(result.is_err()); | |
| } | |
| #[test] | |
| Value::Number(32.0), // Invalid day (out of range) | |
| ]; | |
| let result = native_create_date(args); | |
| assert!(result.is_err()); | |
| } | |
| #[test] | |
| fn test_native_create_date_impossible_date() { | |
| let args = vec![ | |
| Value::Number(2023.0), | |
| Value::Number(2.0), | |
| Value::Number(30.0), // In-range but impossible date (February 30) | |
| ]; | |
| let result = native_create_date(args); | |
| assert!(result.is_err()); | |
| } | |
| #[test] |
| #[test] | ||
| fn test_native_days_between() { | ||
| let date1 = Rc::new(NaiveDate::from_ymd_opt(2023, 10, 20).unwrap()); | ||
| let date2 = Rc::new(NaiveDate::from_ymd_opt(2023, 10, 25).unwrap()); | ||
|
|
||
| // date1 is older, date2 is newer | ||
| let args = vec![ | ||
| Value::Date(date1), | ||
| Value::Date(date2), | ||
| ]; | ||
| let result = native_days_between(args); | ||
| assert!(result.is_ok()); | ||
| if let Value::Number(n) = result.unwrap() { | ||
| assert_eq!(n, 5.0); | ||
| } else { | ||
| panic!("Expected Number variant"); | ||
| } |
There was a problem hiding this comment.
test_native_days_between covers only the case where the second date is later than the first. Since native_days_between uses signed_duration_since, it can legitimately return negative values when the dates are reversed; adding an assertion for the reversed argument order would ensure that signed behavior is covered and won’t regress.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/stdlib/time.rs (1)
238-343: Consider moving these tests totests/time_*to match repository test placement policy.If this repo enforces centralized Rust test placement, keeping these as integration-style files under
tests/will align better with project conventions.
Based on learnings: Place Rust unit and integration tests in thetests/directory with feature-oriented naming (e.g.,file_io_*,crypto_test.rs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/time.rs` around lines 238 - 343, Move the inline module tests out of src/stdlib/time.rs into a new integration test file under tests (e.g., tests/time_date_tests.rs) so they follow the repository's centralized test placement; copy the #[cfg(test)] mod tests block and its test functions (native_today, native_create_date_valid/invalid_month/invalid_day, native_add_days/_negative, native_days_between) into the new file, adapt imports (use crate::stdlib::time::* or public API as needed) and remove the tests module from src/stdlib/time.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stdlib/time.rs`:
- Line 293: The failing CI is due to unformatted Rust test items — run rustfmt
via `cargo fmt --all` (respecting the project's .rustfmt.toml) to normalize the
spacing/formatting around the test attributes (the `#[test]` items introduced in
time.rs) so the three test blocks at those `#[test]` attributes are formatted
correctly; re-run CI after formatting to ensure the diffs disappear.
- Around line 244-250: The test_native_today has a race where expected is
computed after calling native_today causing flakiness at midnight; to fix it
compute expected = Local::now().date_naive() before calling native_today(), then
call native_today() (the function under test), assert result.is_ok(),
pattern-match Value::Date(d) and compare *d to the previously captured expected;
update the code path around test_native_today and the Value::Date unwrap to use
the precomputed expected to eliminate the rollover window.
---
Nitpick comments:
In `@src/stdlib/time.rs`:
- Around line 238-343: Move the inline module tests out of src/stdlib/time.rs
into a new integration test file under tests (e.g., tests/time_date_tests.rs) so
they follow the repository's centralized test placement; copy the #[cfg(test)]
mod tests block and its test functions (native_today,
native_create_date_valid/invalid_month/invalid_day, native_add_days/_negative,
native_days_between) into the new file, adapt imports (use
crate::stdlib::time::* or public API as needed) and remove the tests module from
src/stdlib/time.rs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| fn test_native_today() { | ||
| let result = native_today(vec![]); | ||
| assert!(result.is_ok()); | ||
| if let Value::Date(d) = result.unwrap() { | ||
| let expected = Local::now().date_naive(); | ||
| assert_eq!(*d, expected); | ||
| } else { |
There was a problem hiding this comment.
Stabilize test_native_today against midnight boundary flakiness.
Line 248 computes expected after calling native_today; this can intermittently fail if the date rolls over between those calls.
💡 Suggested fix
#[test]
fn test_native_today() {
+ let before = Local::now().date_naive();
let result = native_today(vec![]);
+ let after = Local::now().date_naive();
assert!(result.is_ok());
if let Value::Date(d) = result.unwrap() {
- let expected = Local::now().date_naive();
- assert_eq!(*d, expected);
+ assert!(*d == before || *d == after);
} else {
panic!("Expected Date variant");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stdlib/time.rs` around lines 244 - 250, The test_native_today has a race
where expected is computed after calling native_today causing flakiness at
midnight; to fix it compute expected = Local::now().date_naive() before calling
native_today(), then call native_today() (the function under test), assert
result.is_ok(), pattern-match Value::Date(d) and compare *d to the previously
captured expected; update the code path around test_native_today and the
Value::Date unwrap to use the precomputed expected to eliminate the rollover
window.
Ran `cargo fmt` to address the CI failures related to formatting in the newly added tests for `src/stdlib/time.rs`. The code is now correctly formatted and CI should pass. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
Verify each finding against the current code and only fix it if needed. Inline comments:
Nitpick comments:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use chrono::NaiveDate; |
There was a problem hiding this comment.
use super::*; already brings NaiveDate into scope from the parent module’s use chrono::{Local, NaiveDate, ...};. The additional use chrono::NaiveDate; re-imports the same name and will fail to compile with E0252 (name defined multiple times). Remove the redundant import or import only the specific items needed instead of super::*.
| use chrono::NaiveDate; |
I have addressed these issues by completely moving the tests out of |
…ition Following PR feedback, the inline tests originally added to `src/stdlib/time.rs` have been removed and appended to the existing integration test file `tests/time_stdlib_test.rs`. The code was correctly formatted using `cargo fmt`, and a midnight-rollover race condition in `test_native_today` was resolved by capturing the expected date before calling the function under test. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verify each finding against the current code and only fix it if needed. Inline comments:
|
Added a test case for |
Added a boundary test checking that `day = 0` correctly yields an error in `native_create_date`, completing the boundary check alongside the `day = 32` invalid case. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let expected = Local::now().date_naive(); | ||
| let result = native_today(vec![]); | ||
| assert!(result.is_ok()); | ||
| if let Value::Date(d) = result.unwrap() { | ||
| assert_eq!(*d, expected); |
There was a problem hiding this comment.
test_native_today can be flaky if it runs across a local midnight boundary: expected is captured before calling native_today(), which itself calls Local::now(). If the date flips between those two calls, the assertion will fail intermittently. Consider capturing before and after around the call and asserting the returned date matches either (or otherwise making the comparison robust to midnight rollover).
| let expected = Local::now().date_naive(); | |
| let result = native_today(vec![]); | |
| assert!(result.is_ok()); | |
| if let Value::Date(d) = result.unwrap() { | |
| assert_eq!(*d, expected); | |
| // Capture the date immediately before calling native_today | |
| let before = Local::now().date_naive(); | |
| let result = native_today(vec![]); | |
| assert!(result.is_ok()); | |
| // Capture the date immediately after calling native_today | |
| let after = Local::now().date_naive(); | |
| if let Value::Date(d) = result.unwrap() { | |
| // Allow for the possibility that midnight passed between the calls | |
| assert!( | |
| *d == before || *d == after, | |
| "native_today returned {d}, which is not equal to either the before ({before}) or after ({after}) date" | |
| ); |
| use chrono::{Local, NaiveDate}; | ||
| use std::rc::Rc; | ||
| use wfl::stdlib::time::{native_add_days, native_create_date, native_days_between, native_today}; | ||
|
|
There was a problem hiding this comment.
Imports are introduced mid-file and duplicate the existing wfl::stdlib::time import. For consistency with other test files, consider consolidating these use statements at the top of the module (and merging the native_create_time import with the other native_* imports).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/time_stdlib_test.rs (2)
91-94: Move imports to the top of the file.Imports for
chrono,std::rc::Rc, and additionalwfl::stdlib::timefunctions are placed mid-file. Consolidate all imports at the top for consistency and readability.🔧 Suggested refactor
use wfl::interpreter::value::Value; use wfl::stdlib::time::native_create_time; +use chrono::{Local, NaiveDate}; +use std::rc::Rc; +use wfl::stdlib::time::{native_add_days, native_create_date, native_days_between, native_today}; #[test] fn test_native_create_time_with_three_args() {Then remove lines 91-94.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/time_stdlib_test.rs` around lines 91 - 94, Move the mid-file imports for chrono::{Local, NaiveDate}, std::rc::Rc, and the wfl::stdlib::time items (native_add_days, native_create_date, native_days_between, native_today) into the file's top-level use/import section and delete the redundant import block currently located near the test code; ensure the compiler still finds those symbols by keeping the same names and paths when consolidating them.
180-194: Add reverse-order test fornative_days_betweento verify signed behavior.The current test only covers the case where
date1 < date2(5 days forward). Since the implementation usessigned_duration_since, a reverse-order test (date1 > date2) would verify it correctly returns negative differences (-5.0).🧪 Suggested test case
#[test] fn test_native_days_between_reverse_order() { let date1 = Rc::new(NaiveDate::from_ymd_opt(2023, 10, 25).unwrap()); let date2 = Rc::new(NaiveDate::from_ymd_opt(2023, 10, 20).unwrap()); // date1 is newer, date2 is older let args = vec![Value::Date(date1), Value::Date(date2)]; let result = native_days_between(args); assert!(result.is_ok()); if let Value::Number(n) = result.unwrap() { assert_eq!(n, -5.0); } else { panic!("Expected Number variant"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/time_stdlib_test.rs` around lines 180 - 194, Add a complementary unit test to verify signed behavior of native_days_between by creating a new test function (e.g., test_native_days_between_reverse_order) that swaps the date arguments so date1 > date2, calls native_days_between with those args, asserts Ok, and checks the returned Value::Number equals -5.0; reference the existing test_native_days_between and the native_days_between function to mirror structure and assertion style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/time_stdlib_test.rs`:
- Around line 96-106: The test test_native_today has a race at midnight because
it calls Local::now().date_naive() separately from native_today; fix by
bracketing the native_today call with two timestamps: capture before =
Local::now().date_naive(), call result = native_today(vec![]), then after =
Local::now().date_naive(); assert result is Ok(Value::Date(d)) and that *d
equals either before or after (handle the midnight boundary when before !=
after). Reference test_native_today, native_today, Local::now().date_naive(),
and Value::Date to locate and update the assertions accordingly.
---
Nitpick comments:
In `@tests/time_stdlib_test.rs`:
- Around line 91-94: Move the mid-file imports for chrono::{Local, NaiveDate},
std::rc::Rc, and the wfl::stdlib::time items (native_add_days,
native_create_date, native_days_between, native_today) into the file's top-level
use/import section and delete the redundant import block currently located near
the test code; ensure the compiler still finds those symbols by keeping the same
names and paths when consolidating them.
- Around line 180-194: Add a complementary unit test to verify signed behavior
of native_days_between by creating a new test function (e.g.,
test_native_days_between_reverse_order) that swaps the date arguments so date1 >
date2, calls native_days_between with those args, asserts Ok, and checks the
returned Value::Number equals -5.0; reference the existing
test_native_days_between and the native_days_between function to mirror
structure and assertion style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| #[test] | ||
| fn test_native_today() { | ||
| let expected = Local::now().date_naive(); | ||
| let result = native_today(vec![]); | ||
| assert!(result.is_ok()); | ||
| if let Value::Date(d) = result.unwrap() { | ||
| assert_eq!(*d, expected); | ||
| } else { | ||
| panic!("Expected Date variant"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition at midnight can cause flaky test.
Both the test (line 98) and native_today (internally) call Local::now().date_naive() separately. If the test runs exactly at midnight, these calls may return different dates, causing spurious failures.
🛡️ Suggested fix: bracket the call with before/after timestamps
#[test]
fn test_native_today() {
- let expected = Local::now().date_naive();
+ let before = Local::now().date_naive();
let result = native_today(vec![]);
+ let after = Local::now().date_naive();
assert!(result.is_ok());
if let Value::Date(d) = result.unwrap() {
- assert_eq!(*d, expected);
+ assert!(
+ *d == before || *d == after,
+ "Expected date to be {before} or {after}, got {d}"
+ );
} else {
panic!("Expected Date variant");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_native_today() { | |
| let expected = Local::now().date_naive(); | |
| let result = native_today(vec![]); | |
| assert!(result.is_ok()); | |
| if let Value::Date(d) = result.unwrap() { | |
| assert_eq!(*d, expected); | |
| } else { | |
| panic!("Expected Date variant"); | |
| } | |
| } | |
| #[test] | |
| fn test_native_today() { | |
| let before = Local::now().date_naive(); | |
| let result = native_today(vec![]); | |
| let after = Local::now().date_naive(); | |
| assert!(result.is_ok()); | |
| if let Value::Date(d) = result.unwrap() { | |
| assert!( | |
| *d == before || *d == after, | |
| "Expected date to be {before} or {after}, got {d}" | |
| ); | |
| } else { | |
| panic!("Expected Date variant"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/time_stdlib_test.rs` around lines 96 - 106, The test test_native_today
has a race at midnight because it calls Local::now().date_naive() separately
from native_today; fix by bracketing the native_today call with two timestamps:
capture before = Local::now().date_naive(), call result = native_today(vec![]),
then after = Local::now().date_naive(); assert result is Ok(Value::Date(d)) and
that *d equals either before or after (handle the midnight boundary when before
!= after). Reference test_native_today, native_today, Local::now().date_naive(),
and Value::Date to locate and update the assertions accordingly.
This PR adds tests to the
stdlib::timemodule fornative_today,native_create_date,native_add_days, andnative_days_betweento fulfill the task of finding an untested part of the codebase and writing passing tests for it.PR created automatically by Jules for task 16849086710152858475 started by @logbie
Summary by CodeRabbit