Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

time: add utimes() and futimes() test #94

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

time: add utimes() and futimes() test #94

wants to merge 2 commits into from

Conversation

ziemleszcz
Copy link
Contributor

@ziemleszcz ziemleszcz commented Feb 21, 2022 •
edited
Loading

JIRA: DTR-276

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: (list targets here).

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

@ziemleszcz ziemleszcz self-assigned this Feb 21, 2022
github-actions[bot]
github-actions bot reviewed Feb 21, 2022
View reviewed changes
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
This was referenced Feb 21, 2022
Merged
Merged
Merged
github-actions[bot]
github-actions bot reviewed Feb 21, 2022
View reviewed changes
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
@ziemleszcz ziemleszcz marked this pull request as ready for review February 21, 2022 15:45
nalajcie
nalajcie reviewed Feb 21, 2022
View reviewed changes
Copy link
Member

@nalajcie nalajcie left a comment

Choose a reason for hiding this comment

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

Nice to have tests with new functionality!

Maybe we can make it a test grup in libc (faster testing, no reboots between the tests) as there are already tests needing filesystem).

To be fair we should test all filesystems (so parameterize FILENAME, but this is also a TODO for resolve_path / symlink tests).

time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
@ziemleszcz ziemleszcz force-pushed the ziemleszcz/utimes branch 2 times, most recently from 12e863a to 444659e Compare February 22, 2022 15:30
github-actions[bot]
github-actions bot reviewed Feb 22, 2022
View reviewed changes
time/test_utimes.c Outdated Show resolved Hide resolved
agkaminski
agkaminski reviewed Feb 22, 2022
View reviewed changes
time/Makefile Outdated Show resolved Hide resolved
time/test_utimes.c Outdated Show resolved Hide resolved
damianloew
damianloew reviewed Nov 25, 2022
View reviewed changes
Copy link
Contributor

@damianloew damianloew left a comment

Choose a reason for hiding this comment

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

@ziemleszcz please let me know whether you can look through the mentioned suggestions and merge the PR.

@mateusz-bloch mateusz-bloch force-pushed the ziemleszcz/utimes branch 2 times, most recently from 1384fcc to b2f3ddf Compare October 30, 2023 19:02
@github-actions
Copy link

github-actions bot commented Oct 30, 2023 •
edited
Loading

Unit Test Results

7 862 tests  +162   7 147 ✅ +162   41m 31s ⏱️ + 2m 46s
  436 suites ±  0     715 💤 ±  0 
    1 files   ±  0       0 ❌ ±  0 

Results for commit 775cec1. ± Comparison against base commit 3a22101.

♻️ This comment has been updated with latest results.

@mateusz-bloch
Copy link
Member

mateusz-bloch commented Nov 8, 2023 •
edited
Loading

@damianloew I followed the documentation below:
utimes: https://pubs.opengroup.org/onlinepubs/7908799/xsh/utimes.html and https://man7.org/linux/man-pages/man2/utime.2.html
what is compatible with POSIX.1-2001

futimes: https://man7.org/linux/man-pages/man3/futimes.3.html
what is compatible with BSD

maska989
maska989 previously approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@maska989 maska989 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but in my opinion I would stay with merge to someone else review also.

damianloew
damianloew requested changes Apr 18, 2024
View reviewed changes
libc/time/utimes.c Outdated Show resolved Hide resolved
libc/time/utimes.c Outdated Show resolved Hide resolved
libc/time/utimes.c Show resolved Hide resolved
libc/time/utimes.c Outdated Show resolved Hide resolved
libc/time/utimes.c Outdated
int64_t adiff_nsec, mdiff_nsec;
#endif

if (test != TEST_SET_PAST) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you have added new cases - I think that this approach with one common function is a good solution - we can add a switch case here to not duplicate code from one and high_value cases.

Copy link
Member

Choose a reason for hiding this comment

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

Due to this, function was get a bit bloated, so I split it into three smaller helper functions

libc/time/utimes.c Outdated Show resolved Hide resolved
libc/time/utimes.c Show resolved Hide resolved
damianloew
damianloew reviewed Apr 18, 2024
View reviewed changes
Copy link
Contributor

@damianloew damianloew left a comment •
edited
Loading

Choose a reason for hiding this comment

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

About the commit message: it should be rather libc: improve utimes/lutimes/futimes tests

@mateusz-bloch mateusz-bloch force-pushed the ziemleszcz/utimes branch 2 times, most recently from 3d13288 to 2bf219c Compare September 17, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateusz-bloch mateusz-bloch mateusz-bloch left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@agkaminski agkaminski agkaminski left review comments

@nalajcie nalajcie nalajcie left review comments

@maska989 maska989 maska989 left review comments

@damianloew damianloew Awaiting requested review from damianloew

Requested changes must be addressed to merge this pull request.

Assignees

@ziemleszcz ziemleszcz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ziemleszcz @mateusz-bloch @nalajcie @agkaminski @maska989 @damianloew