Skip to content

parsedate: clarify time2epoch and add more variations to test 517#21251

Closed
bagder wants to merge 3 commits intomasterfrom
bagder/parsedate
Closed

parsedate: clarify time2epoch and add more variations to test 517#21251
bagder wants to merge 3 commits intomasterfrom
bagder/parsedate

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Apr 7, 2026

Polish the time2epoch function to become a little more readable.

Add a few more curl_getdate() input varations to test 517

@github-actions github-actions bot added the tests label Apr 7, 2026
@testclutch
Copy link
Copy Markdown

Analysis of PR #21251 at 64b113e1:

Test 1093 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 5 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@bagder bagder marked this pull request as ready for review April 7, 2026 10:36
@bagder bagder requested a review from Copilot April 7, 2026 10:36
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors time2epoch() in parsedate.c for readability and expands curl_getdate() coverage in test 517 with additional input variations and edge cases.

Changes:

  • Refactor time2epoch() by extracting leap-day baseline and simplifying the day/seconds computation.
  • Add many additional curl_getdate() test vectors (format variations, timezones, malformed inputs, boundary years).
  • Update .clang-tidy.yml by removing readability-math-missing-parentheses.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
tests/libtest/lib517.c Adds new curl_getdate() parsing test cases (formats, invalid dates, TZ offsets, boundaries).
lib/parsedate.c Makes time2epoch() more readable by splitting computations and naming constants.
.clang-tidy.yml Removes one clang-tidy readability check from the enabled checks list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/parsedate.c Outdated
Comment thread tests/libtest/lib517.c Outdated
Comment thread tests/libtest/lib517.c Outdated
Comment thread .clang-tidy.yml
Comment thread tests/libtest/lib517.c
bagder added 2 commits April 7, 2026 12:55
It's mostly annoying and not helpful
Polish the time2epoch function to become a little more readable.

Add a few more curl_getdate() input varations to test 517
@bagder bagder force-pushed the bagder/parsedate branch from 2d0cf52 to be18657 Compare April 7, 2026 10:58
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Apr 7, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Apr 7, 2026

🤖 Augment PR Summary

Summary: This PR refactors the time2epoch() calculation in lib/parsedate.c to be clearer while preserving its intent, and expands coverage in test 517 for additional curl_getdate() input variants.

Changes:

  • Introduces a named constant for the pre-epoch leap-day offset and simplifies intermediate variables in time2epoch().
  • Adds many new test 517 cases covering relaxed time formatting, out-of-range day-of-month normalization/rejection, timezone offsets, and boundary years (Y2038/2069).
  • Removes the readability-math-missing-parentheses clang-tidy check from the repo configuration.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread tests/libtest/lib517.c Outdated
@bagder bagder requested a review from Copilot April 7, 2026 11:30
@bagder bagder closed this in 3536730 Apr 7, 2026
@bagder bagder deleted the bagder/parsedate branch April 7, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants