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 bug where analytics tries to init in tests which don't import it #3185

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Jan 13, 2023

Hi @andysellick,

Would you be able to approve this if all is OK?

Thanks 👍

What

We want the analytics to load in if it hasn't been already, which is why we check for undefined values, and load in analytics if the values are undefined.

However, our tests do not import load-analytics.js by default, so having a nested check like window.GOVUK.analyticsGa4.vars resulted in an error in our tests, as window.GOVUK.analyticsGa4 didn't exist, so we couldn't run a check on window.GOVUK.analyticsGa4.vars without the JS crashing.

Therefore I have added extra if statements which ensure this code only runs when we can guarantee that load-analytics.js has been imported.

I've checked this against the Jasmine tests in frontend, finder-frontend, and government-frontend and they're now passing.

Why

Without this, tests across our applications to fail, because the DOMContentLoaded code was running on tests which did not have load-analytics.js loaded into the DOM (which is imported only when _layout_for_public is rendered. So window.GOVUK.analyticsGa4 did not exist and then the window.GOVUK.analyticsGa4.vars check would crash.

Visual Changes

None.

@AshGDS AshGDS requested a review from andysellick January 13, 2023 13:50
@AshGDS AshGDS self-assigned this Jan 13, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3185 January 13, 2023 13:50 Inactive
@AshGDS AshGDS force-pushed the fix-failing-tests branch from d4fe702 to 2205bcb Compare January 13, 2023 14:32
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3185 January 13, 2023 14:32 Inactive
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

🚀

@AshGDS AshGDS force-pushed the fix-failing-tests branch from 2205bcb to fa69ba9 Compare January 13, 2023 14:35
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3185 January 13, 2023 14:36 Inactive
@AshGDS AshGDS merged commit c9a3634 into main Jan 13, 2023
@AshGDS AshGDS deleted the fix-failing-tests branch January 13, 2023 14:39
@AshGDS AshGDS mentioned this pull request Jan 13, 2023
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.

3 participants