Run Flamenco Unit Tests in the local timezone #104236

MichaelC · 2023-07-13T23:25:13+02:00

MichaelC commented

2023-07-13 23:25:13 +02:00

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

db30f6f417 Create a mocked clock with time.Local timezone

51868e5918 Merge branch 'main' into mock_test_timezone_aware

Sybren A. Stüvel reviewed 2023-07-22 13:58:49 +02:00

Sybren A. Stüvel left a comment

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

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

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

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

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

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

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
The mock time of 15h04m +7 would be 08h04m in UTC.
In the test assertions it is testing 09h04m which is UTC +1 (Amsterdam in winter I think) which is what timestamp.Local().
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

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

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

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

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.