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.

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

Tests: Don't depend on tester's system timezone for success #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Tests: Don't depend on tester's system timezone for success #8

wants to merge 1 commit into from

Conversation

kisaragi-hiu
Copy link
Contributor

@kisaragi-hiu kisaragi-hiu commented May 6, 2020

  • Org timestamps use tester's timezone, so the ideal value should too
  • ts-format accepts "%z" just like format-time-string, so it's reasonable to expect it to support timezones. Add that into its ideal value.

This should fix #6.

This will make ts-format in its current form fail its test—I think that's justified, as it currently throws away the timestamp's timezone information and inserts the local timezone into its output.

- Org timestamps uses tester's timezone, so the ideal value should too
- ts-format accepts %z just like format-time-string, so it's
  reasonable to expect it to support timezones. Add that into its
  ideal value.
@alphapapa
Copy link
Owner

Thanks.

This will make ts-format in its current form fail its test

Do you mean that this branch fixes one test and breaks another?

@kisaragi-hiu
Copy link
Contributor Author

I mean that it fixes the test, which exposes the fact that ts-format throws away timezones, which makes the test not pass (when system timezone don't happen to line up).

@Levenson
Copy link

@alphapapa Can we merge this and #9? Currently emacs-ts build is broken on Guix, because of the #9.

Tests do not work if you are on a different timezone anyway, so we won't break anything. WDYT?

@alphapapa
Copy link
Owner

Sorry for the delay. See commits linked on #18.

In this PR, adding :tz-offset to the ts-format test in the make-ts call is fine, but changing the other tests to pass seems to paper over the underlying problem, which is that formatting timestamps needs to take the time zone into account and allow it to be specified. I'd rather fix that properly.

alphapapa added a commit that referenced this pull request Jul 5, 2021
@alphapapa
Copy link
Owner

Quoting myself:

In this PR, adding :tz-offset to the ts-format test in the make-ts call is fine

Actually, that doesn't seem to work properly, because the :tz-offset specified in make-ts doesn't affect the resulting timestamp and formatted string. ts-format needs to handle this directly. And the fact that specifying the offset in make-ts doesn't have any effect needs to be documented.

@alphapapa alphapapa self-assigned this Jul 5, 2021
@alphapapa alphapapa added this to the 0.3 milestone Jul 5, 2021
@alphapapa alphapapa modified the milestones: 0.3, 0.4 Aug 22, 2022
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.

Three tests are not entirely timezone-agnostic
3 participants