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

fix(app-project): test the daily stats chart #6399

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Oct 23, 2024

  • test the daily stats chart, on the Classify page. Check that each bar has the expected day and classification count.
  • fix a bug thrown up by those tests, where the bar date wasn't correctly calculated as UTC.
  • fix a bug in the mocks, so that the current daily count matches the count for the current day.
  • fix a bug in the storybook, where the mocks were in reverse order.

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

Package

  • app-project

Linked Issue and/or Talk Post

How to Review

This started out as replacing some tests that were removed in #5210, but those tests failed, because of bugs in the YourStats component. So this now:

  • tests the stats component to check that each day has the correct day name and classification count.
  • fixes the stats calculation so that those tests pass.

To properly test this, you should look at a project that you've classified on in the last week, preferably on more than one day, and check that the daily classification counts are assigned to the correct day.

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

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

Bug Fix

  • The PR creator has listed user actions to use when testing if bug is fixed
  • The bug is fixed
  • Unit tests are added or updated

- test the daily stats chart, on the Classify page. Check that each bar has the expected day and classification count.
- fix a bug thrown up by those tests, where the bar date wasn't correctly calculated as UTC.
- fix a bug in the mocks, so that the current daily count matches the count for the current day.
@coveralls
Copy link

coveralls commented Oct 23, 2024

Coverage Status

coverage: 77.78% (+0.003%) from 77.777%
when pulling 81f046c on eatyourgreens:test-your-stats
into 6a82afb on zooniverse:master.

Comment on lines +35 to 44
describe('Component > YourStats > Daily bars', function () {
YourStatsStoreMock.user.personalization.stats.thisWeek.forEach(function (count, i) {
it(`should have an accessible description for ${count.longLabel}`, function () {
const clock = sinon.useFakeTimers(new Date('2019-10-01T19:00:00+06:00'))
const YourStatsStory = composeStory(YourStats, Meta)
render(<YourStatsStory />)
const bar = screen.getByRole('img', { name: count.alt })
expect(bar).to.exist()
clock.restore()
})
Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 23, 2024

Choose a reason for hiding this comment

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

This goes into its own block so that the mocked clock doesn't interfere with other tests. For example, if you put the mocked clock into a before block, it stops the animation in the project stats container tests, causing them to hang.

There might be some GitHub actions running for ~6 hours as a result of that.

Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 23, 2024

Choose a reason for hiding this comment

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

I've chosen a mocked time where the day name/number in the local clock is different from the day name/number in UTC, in order to try and trigger #2244. The local day is Tuesday 1 October but the UTC day is Wednesday 2 October. The rendered classification count should be shown for Tuesday.

@@ -14,7 +14,9 @@ function DailyClassificationsChartContainer({
}) {
const TODAY = new Date()
const stats = thisWeek.map(({ count: statsCount, period }) => {
const day = new Date(period)
Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 23, 2024

Choose a reason for hiding this comment

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

This gives the wrong day when the date is a single digit, I think because a date like 2019-10-2 isn't a valid ISO date (it should be 2019-10-02.) Breaking the date into parts, then constructing a UTC date from year, month index, and date, seems to work better.

@eatyourgreens eatyourgreens changed the title fix(app-project): test daily stats chart fix(app-project): test the daily stats chart Oct 23, 2024
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

Thanks!

I went further with this refactor and removed .defaultProps() from YourStats and the unused, legacy mockStore from YourStatsContainer.

@goplayoutside3
Copy link
Contributor

I also noticed during review the YourStats component still displays "total" from user.project_preferences.activity_count instead of ERAS, meaning the total displayed on a project page will differ from a user's homepage or stats page. I'll open an Issue and PR to fix.

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.

Daily count stats bug YourStats story is broken in Safari
3 participants