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

Add ability to track browser errors #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

csutter
Copy link
Contributor

@csutter csutter commented Oct 31, 2024

A recent incident on Finder Frontend saw the filters on search pages malfunctioning due to an error loading an asset file (which hadn't been uploaded correctly). We could write a specific e2e test exercising the filters, but ideally this functionality should already be tested extensively in the app's own integration/system tests.

Instead, this adds a helper to enhance a test suite by keeping track of console errors, Javascript exceptions, and network issues, which should be able to catch many sources of unexpected failures in our frontend code.

  • Add trackBrowserErrors helper library
  • Use trackBrowserErrors in the Finder Frontend e2e tests to begin with (possibly with a view to other apps opting in to this behaviour too)

In the browser console

image

As part of the Playwright test

image

A recent incident on Finder Frontend saw the filters on search pages
malfunctioning due to an error loading an asset file (which hadn't been
uploaded correctly). We could write a specific e2e test exercising the
filters, but ideally this functionality should already be tested
extensively in the app's own integration/system tests.

Instead, this adds a helper to enhance a test suite by keeping track of
console errors, Javascript exceptions, and network issues, which should
be able to catch many sources of unexpected failures in our frontend
code.

- Add `trackBrowserErrors` helper library
- Use `trackBrowserErrors` in the Finder Frontend e2e tests to begin
  with (possibly with a view to other apps opting in to this behaviour
  too)
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Nice ⭐ Will leave it for someone from Platform Engineering to approve.

(I imagine they'll have thoughts as to how/when to pull this module into the other application test files, and perhaps how to ensure it doesn't get missed from any given app test file)

@csutter
Copy link
Contributor Author

csutter commented Oct 31, 2024

(I imagine they'll have thoughts as to how/when to pull this module into the other application test files, and perhaps how to ensure it doesn't get missed from any given app test file)

Yeah - I've intentionally added it to Finder Frontend only for now, because we probably would want to check whether any apps for obscure legacy reasons might have JS errors/tech debt that we're aware of. 👍

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks great, well done working this out so quickly.

I guess the thing that is a bit concerning is if there's any external resources that could lead to an alert or not as they could be annoying. It's a shame we don't know for sure if Smokey had roughly the same functionality (I think it did) as that'd be reason to be confident.

});
};

export { trackBrowserErrors };
Copy link
Member

Choose a reason for hiding this comment

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

I'd wonder whether this file should be unit tested? I guess we've not got anything of substance in the lib directory already to warrant it so probably a lack of precedent.


test.describe("Finder frontend", { tag: ["@app-finder-frontend"] }, () => {
trackBrowserErrors(test);
Copy link
Member

Choose a reason for hiding this comment

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

Do we know of a way we can apply this to everything? I guess the equivalent of a beforeEach for everything in the suite?

There is setup files - an example is the auth.setup.js -perhaps those type of files offer a hook to do a global beforeEach.

@theseanything
Copy link
Contributor

Thanks @csutter for looking at this!

We could write a specific e2e test exercising the filters, but ideally this functionality should already be tested extensively in the app's own integration/system tests.

Why not have this as a E2E/smoke test? Sounds like important user functionality that would be good to validate.

We really shouldn't have assertions for testing non-user functionality. They aren't testing what we actually care about. Testing JS/console failures have in the past been a source of flakey-ness and don't always correlate broken user functionality.

@theseanything
Copy link
Contributor

@csutter wondering if you're still working on this or can this PR be closed?

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.

4 participants