test(app-project): run YourStats tests in a non-UTC timezone #6412

eatyourgreens · 2024-10-29T08:58:12Z

A couple of small refactors for the YourStats tests:

add missing timestamps to the mock ERAS data, to match real ERAS responses.
~~test the weekly stats in the -05:00 timezone, to check for timezone bugs.~~ this doesn’t work. Tests always run in the OS time zone.
run the tests in the -06:00:00 timezone by setting the Node TZ env var, to catch Daily count stats bug #2244.
use getUTCDay to get the day number for daily stats, fixing Daily count stats bug #2244.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

app-project

Linked Issue and/or Talk Post

Daily count stats bug #2244.

How to Review

I'm not seeing any errors when I run this, but I'm running it on a computer that's set to UTC. I think it might need to be run in a different time zone, in order to test it. Whatever timezone it's run in, the stats chart should always start on Monday.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

coveralls · 2024-10-29T09:04:54Z

coverage: 77.651% (+0.02%) from 77.635%
when pulling 2ba3612 on eatyourgreens:test-your-stats
into 860f09f on zooniverse:master.

Tests don't pass locally

goplayoutside3 · 2024-10-29T13:56:01Z

I read through the code and approved, but then I ran the tests locally on my local machine which is set to US Central Time. These tests don't pass

goplayoutside3 · 2024-10-29T14:01:05Z

I'm not seeing any errors when I run this, but I'm running it on a computer that's set to UTC

You can test this by changing your local machine's time settings. I just set mine to Monday Oct 28 UTC and the tests pass locally. They do not pass when mine is set to today's date and Chicago time.

eatyourgreens · 2024-10-29T16:35:14Z

I read through the code and approved, but then I ran the tests locally on my local machine which is set to US Central Time. These tests don't pass.

Yep, these tests catch the bug that you found in #2244, which doesn't trigger when you run the code in a UTC time zone.

goplayoutside3 · 2024-10-29T17:01:03Z

See #2244 (comment). Reviewing this PR is on hold until design decisions are made about YourStats.

eatyourgreens · 2024-10-29T17:14:46Z

@goplayoutside3 it works now when it's run in CDT, but there doesn't seem to be an easy way to check that in CI.

EDIT: This version passes in Guam, Kuala Lumpur, London, and Chicago.

eatyourgreens · 2024-10-29T17:15:57Z

packages/app-project/stores/User/UserPersonalization/YourStats/YourStats.js

@@ -74,7 +80,7 @@ const YourStats = types
        weekDay.setUTCDate(newDate)
        const period = weekDay.toISOString().substring(0, 10)
        const { count } = dailyCounts.find(count => count.period.startsWith(period)) || { count: 0, period }
-        const dayNumber = weekDay.getDay()
+        const dayNumber = weekDay.getUTCDay()


This was causing the weekday bug in #2244, but it only triggers when you run getDay() in a time zone other than UTC. Otherwise, getDay() is identical to getUTCDay().

See #2528 for some context on why the code is getting the weekday here.

eatyourgreens · 2024-10-29T17:41:01Z

packages/app-project/stores/User/UserPersonalization/YourStats/YourStats.spec.js

-      clock = sinon.useFakeTimers({ now: new Date(2019, 9, 1, 12), toFake: ['Date'] })
+      // Set the local clock to 1am on Tuesday 1 October 2019, UTC.
+      // Stats should be shown for Monday 30 September 2019, local time.
+      clock = sinon.useFakeTimers(new Date('2019-09-30T20:00:00-05:00'))


This won't set the clock timezone to -05:00. new Date() always runs in the time zone of the machine that's running the code.

Methods like date.getDay() always return values in the OS time zone too.

eatyourgreens · 2024-11-01T20:00:48Z

This is useful. How to run Node tests in different time zones, by setting the TZ environment variable at runtime.

https://www.stefanjudis.com/today-i-learned/set-the-default-time-zone-in-node-js/

eatyourgreens · 2024-11-05T09:03:29Z

I've updated this to catch #2244 by running the tests in the -06:00:00 time zone. CI tests now catch the bug and fail.
https://github.com/zooniverse/front-end-monorepo/actions/runs/11680689280/job/32524223518?pr=6412

I've then updated the code to pass the tests. Test-driven development FTW!

eatyourgreens · 2024-11-05T09:08:24Z

packages/app-project/stores/User/UserPersonalization/YourStats/YourStats.spec.js

+      { count: 12, period: '2019-09-29T00:00:00.000Z' },
+      { count: 12, period: '2019-09-30T00:00:00.000Z' },
+      { count: 13, period: '2019-10-01T00:00:00.000Z' },
+      { count: 14, period: '2019-10-02T00:00:00.000Z' },
+      { count: 10, period: '2019-10-03T00:00:00.000Z' },
+      { count: 11, period: '2019-10-04T00:00:00.000Z' },
+      { count: 8, period: '2019-10-05T00:00:00.000Z' },
+      { count: 15, period: '2019-10-06T00:00:00.000Z' }


These mocks should match the API responses for real ERAS data, which uses midnight UTC to mark the start of each day.

eatyourgreens · 2024-11-05T09:11:57Z

...ponents/YourStats/components/DailyClassificationsChart/DailyClassificationsChartContainer.js

@@ -17,7 +17,7 @@ function DailyClassificationsChartContainer({
    const [year, monthIndex, date] = period.split('-')
    const utcDay = Date.UTC(year, monthIndex - 1, date)
    const day = new Date(utcDay)
-    const isToday = day.getUTCDay() === TODAY.getDay()
+    const isToday = day.getUTCDay() === TODAY.getUTCDay()


"Today" is always the current day in Greenwich.

A couple of small refactors for the `YourStats` tests: - add missing timestamps to the mock ERAS data, to match real ERAS responses. - test the weekly stats in the `-05:00` timezone, to check for timezone bugs.

eatyourgreens force-pushed the test-your-stats branch from 4fdec87 to 52802f9 Compare October 29, 2024 08:58

eatyourgreens mentioned this pull request Oct 29, 2024

Daily count stats bug #2244

Open

goplayoutside3 previously approved these changes Oct 29, 2024

View reviewed changes

eatyourgreens commented Oct 29, 2024

View reviewed changes

eatyourgreens force-pushed the test-your-stats branch from 772623b to 1bd3d43 Compare October 29, 2024 17:24

eatyourgreens commented Oct 29, 2024

View reviewed changes

eatyourgreens changed the title ~~test(app-project): refactor YourStats tests to a non-UTC timezone~~ test(app-project): run YourStats tests in a non-UTC timezone Nov 5, 2024

eatyourgreens force-pushed the test-your-stats branch 2 times, most recently from b5f0770 to 55d4d50 Compare November 5, 2024 08:56

eatyourgreens commented Nov 5, 2024

View reviewed changes

eatyourgreens added 5 commits November 6, 2024 16:17

test(app-project): refactor YourStats tests to a non-UTC timezone

2104465

A couple of small refactors for the `YourStats` tests: - add missing timestamps to the mock ERAS data, to match real ERAS responses. - test the weekly stats in the `-05:00` timezone, to check for timezone bugs.

calculate the first day of the week in UTC

1eb7b17

Document firstDayOfWeek

d703a43

Run the tests in the -06:00 timezone

c7c6246

Use UTC times consistently for daily classifications

2ba3612

eatyourgreens force-pushed the test-your-stats branch from 15031c7 to 2ba3612 Compare November 6, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(app-project): run YourStats tests in a non-UTC timezone #6412

test(app-project): run YourStats tests in a non-UTC timezone #6412

eatyourgreens commented Oct 29, 2024 •

edited

Loading

coveralls commented Oct 29, 2024 •

edited

Loading

goplayoutside3 commented Oct 29, 2024

goplayoutside3 commented Oct 29, 2024

eatyourgreens commented Oct 29, 2024

goplayoutside3 commented Oct 29, 2024 •

edited

Loading

eatyourgreens commented Oct 29, 2024 •

edited

Loading

eatyourgreens Oct 29, 2024 •

edited

Loading

eatyourgreens Oct 29, 2024

eatyourgreens Oct 29, 2024

eatyourgreens Oct 29, 2024

eatyourgreens commented Nov 1, 2024

eatyourgreens commented Nov 5, 2024

eatyourgreens Nov 5, 2024 •

edited

Loading

eatyourgreens Nov 5, 2024

test(app-project): run YourStats tests in a non-UTC timezone #6412

Are you sure you want to change the base?

test(app-project): run YourStats tests in a non-UTC timezone #6412

Conversation

eatyourgreens commented Oct 29, 2024 • edited Loading

Package

Linked Issue and/or Talk Post

How to Review

Checklist

General

coveralls commented Oct 29, 2024 • edited Loading

goplayoutside3 commented Oct 29, 2024

goplayoutside3 commented Oct 29, 2024

eatyourgreens commented Oct 29, 2024

goplayoutside3 commented Oct 29, 2024 • edited Loading

eatyourgreens commented Oct 29, 2024 • edited Loading

eatyourgreens Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Oct 29, 2024

Choose a reason for hiding this comment

eatyourgreens Oct 29, 2024

Choose a reason for hiding this comment

eatyourgreens Oct 29, 2024

Choose a reason for hiding this comment

eatyourgreens commented Nov 1, 2024

eatyourgreens commented Nov 5, 2024

eatyourgreens Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Nov 5, 2024

Choose a reason for hiding this comment

eatyourgreens commented Oct 29, 2024 •

edited

Loading

coveralls commented Oct 29, 2024 •

edited

Loading

goplayoutside3 commented Oct 29, 2024 •

edited

Loading

eatyourgreens commented Oct 29, 2024 •

edited

Loading

eatyourgreens Oct 29, 2024 •

edited

Loading

eatyourgreens Nov 5, 2024 •

edited

Loading