studio/flamenco
studio/flamenco
12
30
Fork 38
Code Issues 63 Pull Requests 5 Releases Wiki Activity

Run Flamenco Unit Tests in the local timezone #104236

Manually merged
Sybren A. Stüvel merged 2 commits from michael-2/flamenco:mock_test_timezone_aware into main 2023-08-23 16:08:37 +02:00
Conversation 4 Commits 2 Files Changed 3 +8 -7
MichaelC commented 2023-07-13 23:25:13 +02:00
Contributor
Copy Link

Resolves having to specify Europe/Amsterdam timezone before running Flamenco mocked-clock tests.
#104219

The solution uses Go time.Local timezone to satisfy unit tests assertions using a Mock clock. The timezone of the local workstation running the tests.

Tested by setting the local workstation to two different timezones to America/New_York and Europe/Amsterdam.
timezone_make_test.txt

Resolves having to specify Europe/Amsterdam timezone before running Flamenco mocked-clock tests. https://projects.blender.org/studio/flamenco/issues/104219 The solution uses Go time.Local timezone to satisfy unit tests assertions using a Mock clock. The timezone of the local workstation running the tests. Tested by setting the local workstation to two different timezones to America/New_York and Europe/Amsterdam. [timezone_make_test.txt](/attachments/bcf9e003-19fe-4b6a-894f-0c01672fbadd)
timezone_make_test.txt
6.7 KiB
MichaelC added 2 commits 2023-07-13 23:25:15 +02:00
Sybren A. Stüvel reviewed 2023-07-22 13:58:49 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Thanks for the PR. I'm wondering if this is the right approach, though. If there is code in Flamenco that makes assumptions about the timezones, then I feel that that's a bug that needs fixing.

Thanks for the PR. I'm wondering if this is the right approach, though. If there is code in Flamenco that makes assumptions about the timezones, then I feel that that's a bug that needs fixing.
internal/manager/job_compilers/job_compilers_test.go
@ -250,13 +251,13 @@ func TestSimpleBlenderRenderOutputPathFieldReplacement(t *testing.T) {
require.NotNil(t, aj)
// The job compiler should have replaced the {timestamp} and {ext} fields.
assert.Equal(t, "/root/2006-01-02_090405/jobname/######", aj.Settings["render_output_path"])
Sybren A. Stüvel commented 2023-07-22 13:58:46 +02:00
Owner
Copy Link

This was intentionally using a mocked clock in a different timezone, and ensuring that the created timestamp is in UTC. By moving this all to the local timezone, the test semantics are changed.

This was intentionally using a mocked clock in a different timezone, and ensuring that the created timestamp is in UTC. By moving this all to the local timezone, the test semantics are changed.
MichaelC commented 2023-07-22 17:29:39 +02:00
Author
Contributor
Copy Link

I did wonder about that but moved it to local timezone. I didn't realise it was intentionally a timezone test. I can fix that back up to test UTC timestamps.

I did wonder about that but moved it to local timezone. I didn't realise it was intentionally a timezone test. I can fix that back up to test UTC timestamps.
dr.sybren marked this conversation as resolved
MichaelC commented 2023-07-22 17:32:04 +02:00
Author
Contributor
Copy Link

Thanks for the PR. I'm wondering if this is the right approach, though. If there is code in Flamenco that makes assumptions about the timezones, then I feel that that's a bug that needs fixing.

I'd assumed by the issue/bug description that these tests were supposed to run independently of timezones. I dont think Flamenco is timezone aware or has a timezone bug... but the tests seemed to be hard coded to Amsterdam (+7)... and thought that was the problem in the issue you raised. Is this the wrong assumption?

> Thanks for the PR. I'm wondering if this is the right approach, though. If there is code in Flamenco that makes assumptions about the timezones, then I feel that that's a bug that needs fixing. I'd assumed by the issue/bug description that these tests were supposed to run independently of timezones. I dont think Flamenco is timezone aware or has a timezone bug... but the tests seemed to be hard coded to Amsterdam (+7)... and thought that was the problem in the issue you raised. Is this the wrong assumption?
Sybren A. Stüvel commented 2023-07-24 11:28:31 +02:00
Owner
Copy Link

I'd assumed by the issue/bug description that these tests were supposed to run independently of timezones.

They are supposed to work regardless of which timezone the computer that runs them is in.

the tests seemed to be hard coded to Amsterdam (+7)

I'm in Amsterdam, The Netherlands. Not sure which Amsterdam is in +7, but the local time here is UTC+2 (at least now, in summertime). I wrote the test to use a different-than-local timezone intentionally.

> I'd assumed by the issue/bug description that these tests were supposed to run independently of timezones. They are supposed to work regardless of which timezone the computer that runs them is in. > the tests seemed to be hard coded to Amsterdam (+7) I'm in Amsterdam, The Netherlands. Not sure which Amsterdam is in +7, but the local time here is UTC+2 (at least now, in summertime). I wrote the test to use a different-than-local timezone intentionally.
MichaelC commented 2023-08-05 04:51:45 +02:00
Author
Contributor
Copy Link

I've looked a bit more at the code base with your hint that this is supposed to be testing that the output path directory name uses UTC timestamp, and not a local timestamp.

With this context... I think the following is true...

  1. The javascript function defined in js_globals.go uses timestamp.Local() and it appears this is the function responsible for inserting the timestamp in the output path for 'timestamp' variable. https://projects.blender.org/studio/flamenco/src/branch/main/internal/manager/job_compilers/js_globals.go#L33
  2. The mock time of 15h04m +7 would be 08h04m in UTC.
  3. In the test assertions it is testing 09h04m which is UTC +1 (Amsterdam in winter I think) which is what timestamp.Local().
  4. Flamenco doesn't appear to be timezone aware/dependent.

If I swap timestamp.Local() for timestamp.UTC() then the output folder path is in UTC and if the assertion checks against 08h04m (instead of 09h04m) this makes sense and the test passes. I dont think this is the right change though.

Instead, this javascript jsFormatTimestampLocal() should be local date and time to simplify the end-user experience looking for render output paths.

So I would expect that the test case needs check against the local timestamp and not UTC.

Am I on the right path here?

I've looked a bit more at the code base with your hint that this is supposed to be testing that the output path directory name uses UTC timestamp, and not a local timestamp. With this context... I think the following is true... 1. The javascript function defined in js_globals.go uses timestamp.Local() and it appears this is the function responsible for inserting the timestamp in the output path for 'timestamp' variable. https://projects.blender.org/studio/flamenco/src/branch/main/internal/manager/job_compilers/js_globals.go#L33 2. The mock time of 15h04m +7 would be 08h04m in UTC. 3. In the test assertions it is testing 09h04m which is UTC +1 (Amsterdam in winter I think) which is what timestamp.Local(). 4. Flamenco doesn't appear to be timezone aware/dependent. If I swap timestamp.Local() for timestamp.UTC() then the output folder path is in UTC and if the assertion checks against 08h04m (instead of 09h04m) this makes sense and the test passes. I dont think this is the right change though. Instead, this javascript jsFormatTimestampLocal() should be local date and time to simplify the end-user experience looking for render output paths. So I would expect that the test case needs check against the local timestamp and not UTC. Am I on the right path here?
Sybren A. Stüvel commented 2023-08-23 15:30:37 +02:00
Owner
Copy Link

Thanks for your analysis.

Instead, this javascript jsFormatTimestampLocal() should be local date and time to simplify the end-user experience looking for render output paths.

I agree, using local timestamps in the paths is indeed easier for end users, and simplicity is king.

Thanks for your analysis. > Instead, this javascript jsFormatTimestampLocal() should be local date and time to simplify the end-user experience looking for render output paths. I agree, using local timestamps in the paths is indeed easier for end users, and simplicity is king.
Sybren A. Stüvel approved these changes 2023-08-23 15:59:32 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

The code looks good to me!

I've added a few inline comments, I'll address those while landing the PR. Thanks again!

The code looks good to me! I've added a few inline comments, I'll address those while landing the PR. Thanks again!
internal/manager/job_compilers/job_compilers_test.go
@ -58,3 +58,3 @@
func mockedClock(t *testing.T) clock.Clock {
c := clock.NewMock()
now, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05+07:00")
//current_location, err := time.LoadLocation("Local")
Sybren A. Stüvel commented 2023-08-23 15:56:54 +02:00
Owner
Copy Link

Please remove the commented-out code.

Please remove the commented-out code.
internal/manager/sleep_scheduler/sleep_scheduler_test.go
@ -270,3 +270,3 @@
mockedClock := clock.NewMock()
mockedNow, err := time.Parse(time.RFC3339, "2022-06-07T11:14:47+02:00")
mockedNow, err := time.ParseInLocation("2006-01-02T15:04:05", "2022-06-07T11:14:47", time.Local)
Sybren A. Stüvel commented 2023-08-23 15:58:00 +02:00
Owner
Copy Link

The sleep scheduler should work well (that is, always treat the schedule as local time), regardless of whether the mocked clock is set to local time or not. I've taken the liberty of addressing this myself in eb9f46dc9b, so this change can be removed from this PR.

The sleep scheduler should work well (that is, always treat the schedule as local time), regardless of whether the mocked clock is set to local time or not. I've taken the liberty of addressing this myself in eb9f46dc9b5212e99d44899e0c298f17610f2a24, so this change can be removed from this PR.
Sybren A. Stüvel manually merged commit bc7b434121 into main 2023-08-23 16:08:37 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
Good First Issue
  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Status
Archived

  
Status
Confirmed

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Job Type
Custom Flamenco job type

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Good First Issue
Priority
High
Priority
Low
Priority
Normal
Status
Archived
Status
Confirmed
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Job Type
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: studio/flamenco#104236
No description provided.
Delete Branch "michael-2/flamenco:mock_test_timezone_aware"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?