Skip to content

Add deterministic test & extract time formatter for list_files (#616)#619

Open
octoaide[bot] wants to merge 3 commits intomainfrom
octoaide/issue-616-2026-04-28T00-25-31
Open

Add deterministic test & extract time formatter for list_files (#616)#619
octoaide[bot] wants to merge 3 commits intomainfrom
octoaide/issue-616-2026-04-28T00-25-31

Conversation

@octoaide
Copy link
Copy Markdown
Contributor

@octoaide octoaide Bot commented Apr 28, 2026

Closes #616

Summary

Prepare tests for the upcoming chrono -> jiff migration by isolating the time formatting logic and adding a deterministic test for list_files modified-time output.

What I changed

  • Extracted a helper format_modified_time(SystemTime) -> String in src/root/ifconfig.rs to centralize the local-time formatting (currently using chrono with format "%Y/%m/%d %T"). This makes it easy to replace the implementation later with jiff without modifying list_files logic.
  • Updated list_files to call the new helper, simplifying the loop body.
  • Added test list_files_formats_modified_time_in_local_timezone that:
    • Sets TZ=UTC (using the existing pattern with unsafe std::env::set_var) so local-time formatting is deterministic in CI and locally.
    • Uses hard-coded expected strings for fixed timestamps (UNIX_EPOCH and UNIX_EPOCH + 1_700_000_000s) instead of computing expected values with a time crate.
    • Does not introduce chrono or jiff just for test expectations.

Notes and verification

  • Only src/root/ifconfig.rs was modified.
  • Cargo.toml is unchanged.
  • Tests were run locally: cargo test list_files (and full test & clippy runs). All relevant tests pass.

Why

This change prepares the codebase for a safe swap from chrono to jiff by:

  • Isolating the formatting logic into a single function so the swap is localized to one place.
  • Adding a deterministic test that asserts the exact local-time formatted output (YYYY/MM/DD HH:MM:SS) using fixed expected strings, preventing regressions.

Closes #616.

@octoaide octoaide Bot requested review from sehkone and zmrdltl April 28, 2026 00:31
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.90%. Comparing base (f462850) to head (9b8366e).

Files with missing lines Patch % Lines
src/root/ifconfig.rs 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
- Coverage   80.99%   80.90%   -0.09%     
==========================================
  Files          27       27              
  Lines        3977     3954      -23     
==========================================
- Hits         3221     3199      -22     
+ Misses        756      755       -1     

☔ 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.

Copy link
Copy Markdown
Contributor

@zmrdltl zmrdltl left a comment

Choose a reason for hiding this comment

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

Please revert the production refactor and keep this PR focused on adding the deterministic list_files modified-time output test.

For example:

const LIST_FILES_MTIME_CHILD_ENV: &str = "ROXY_LIST_FILES_MTIME_CHILD";
...
#[test]
fn list_files_formats_modified_time_in_local_timezone() {
    if std::env::var_os(LIST_FILES_MTIME_CHILD_ENV).is_some() {
        let dir = make_temp_test_dir("list-mtime");
        let file_path = dir.join("fixed.yaml");
        write_text_file(&file_path, "fixed");

        let ts = UNIX_EPOCH + Duration::from_secs(1_700_000_000);
        let file = OpenOptions::new()
            .write(true)
            .open(&file_path)
            .expect("test file should be openable for timestamp update");
        file.set_modified(ts)
            .expect("test file modified time should be settable");

        let listed = list_files(
            dir.to_str().expect("temp path should be valid utf-8"),
            None,
            false,
        )
        .expect("listing files should succeed");
        assert_eq!(
            listed,
            vec![(
                5,
                "2023/11/14 22:13:20".to_string(),
                "fixed.yaml".to_string()
            )]
        );

        fs::remove_dir_all(&dir).expect("temp test directory should be removable");
        return;
    }

    let output = std::process::Command::new(
        std::env::current_exe().expect("current test executable should be available"),
    )
    .arg("list_files_formats_modified_time_in_local_timezone")
    .env(LIST_FILES_MTIME_CHILD_ENV, "1")
    .env("TZ", "UTC")
    .output()
    .expect("child test process should run");

    assert!(
        output.status.success(),
        "child test process should pass\nstdout:\n{}\nstderr:\n{}",
        String::from_utf8_lossy(&output.stdout),
        String::from_utf8_lossy(&output.stderr)
    );
}

@octoaide
Copy link
Copy Markdown
Contributor Author

octoaide Bot commented Apr 28, 2026

Thanks — I reviewed your comment and updated the PR accordingly.

I reverted the production refactor and restored the original inline DateTime formatting in list_files (removed the format_modified_time helper and dropped the time::SystemTime import). I also replaced the test with the child-process pattern you suggested: the parent re-execs the test binary with TZ=UTC and ROXY_LIST_FILES_MTIME_CHILD=1, and the child branch creates a temp file, sets its mtime to 1_700_000_000, and asserts that list_files returns the expected tuple (5, "2023/11/14 22:13:20", "fixed.yaml").

I've committed and pushed these changes. I ran the tests and cargo clippy — all three list_files tests pass and clippy is clean.

Thanks for the guidance — this PR is now focused on the deterministic modified-time test as requested.

@sae-bom sae-bom removed the request for review from sehkone April 29, 2026 10:14
@sae-bom
Copy link
Copy Markdown
Contributor

sae-bom commented Apr 30, 2026

@zmrdltl Please update the direction of this issue based on the comments on the parent issue.

@zmrdltl
Copy link
Copy Markdown
Contributor

zmrdltl commented Apr 30, 2026

@sae-bom I updated #616 to match the direction from the parent issue discussion. Once this work is completed, the other sibling issues will no longer be needed, so I’ll close them as superseded.

@zmrdltl
Copy link
Copy Markdown
Contributor

zmrdltl commented Apr 30, 2026

@octoaide Please update the work to match the revised direction in #616.

@octoaide octoaide Bot force-pushed the octoaide/issue-616-2026-04-28T00-25-31 branch from c5b6564 to 11dcc66 Compare May 4, 2026 20:31
@octoaide
Copy link
Copy Markdown
Contributor Author

octoaide Bot commented May 4, 2026

I reviewed the direction from #616 and pivoted the PR to match it (per @zmrdltl). Instead of preparing a chrono→jiff swap, I removed direct chrono usage entirely and simplified the API where chrono had been needed.

What I changed (all committed and pushed):

  • list_files now returns Vec (just file names). Callers never used size or modified-time, so keeping only names simplifies things.
  • load_netplan_yaml and NetplanYaml::apply were updated to consume the simpler return type.
  • Removed the format_modified_time helper and the related test (they're no longer applicable).
  • Removed chrono from Cargo.toml.
  • Added an Unreleased entry to CHANGELOG.

Verification:

  • cargo build, cargo test, and cargo clippy --all-targets -- -D warnings all pass.
  • No direct chrono usage remains in Cargo.toml, src/, or tests/.

Thanks for the guidance — I’ve updated the work to follow the revised direction and confirmed everything is addressed.

@sae-bom sae-bom requested a review from zmrdltl May 6, 2026 10:13
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.

Simplify list_files and remove chrono

2 participants