search

department-of-veterans-affairs / va.gov-team

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
282 stars 203 forks source link

Platform Cypress tests discovery #32397

Closed theodur closed 2 years ago

theodur commented 2 years ago

Details

As part of building only changed apps in CI, we need to ensure that only the necessary Cypress tests are executed. Currently, all Cypress tests in the src/platform directory are executed in any PR. This is fine for full builds, but becomes problematic in only changed apps builds, because many platform Cypress tests depend on other apps being built for their tests to run. This causes many of them to fail. An example run can be seen in this commit's GHA workflow: https://github.com/department-of-veterans-affairs/vets-website/runs/3971909073?check_suite_focus=true

This ticket is for researching which of the platform Cypress tests should run in only changed apps builds, and what dependent apps if any, need to be built in correlation.

To do

theodur commented 2 years ago

The Cypress tests in the src/platform directory of the vets-website repo are separated into the following parts:

Forms System

These are tests used for testing the forms system. They mostly use the hca app for testing. This means that in order for these to pass in only changed apps builds, the hca app would need to be built alongside the other changed apps.

Site Wide

Tests in this directory usually test some component of the site that is used more broadly, like the header or facility sidebar.

User

There is only one Cypress in this category. This test is related to user functionalities like signing in and authentication

theodur commented 2 years ago

Recommendation

Tests that shouldn't be run

The forms system tests shouldn't need to be run for only changed apps builds. The chances of changes to app code affecting the forms system is slim to none. This was an issue when the forms system imported a few form definitions from other apps, but those definitions have been moved out of the apps and into the forms system in the cross app import work.

The unauthenticated user test also probably doesn't need to be run in only changed apps builds. The authentication functionality that apps use is separate from the apps themselves, therefore there shouldn't be any reason why changes to an app's code would affect authentication at a site-wide level. Because this test checks unauthenticated functionality using a few arbitrary apps, one alternative would be to dynamically use the changed app in CI to run this test on. The only caveat in that is that not all apps in the repo require authentication, so if an only changed apps build in CI was for an app like the coronavirus-screener, this test would fail.

In the site wide platform tests, there are many tests that probably don't need to be run:

Tests that should be run

The test that probably should be run in only changed apps builds is the Mega menu test: src/platform/site-wide/mega-menu/tests/megaMenu.cypress.spec.js. This is an overall test of the header that ensures it is being rendered properly with the correct menus. This would be useful to test to ensure that app changes won't interfere with the header in any unexpected ways. This test should also be enough to cover the situations mentioned above with other site-wide tests, because app changes shouldn't affect their functionality, and if app changes did, it would most likely be because of issues with rendering the header, which this test should cover.

Currently, the Mega menu test is run on the homepage. This works fine when the static-pages app is built, since that's the only app asset that's loaded on non-app pages, like the homepage. When the static-pages app isn't built, this test will fail, because the homepage doesn't have any application asset to load the header from. The header is injected into app assets from the startApp function, which is why every app is able to load the header on their page. This means that the mega menu test can be run on app URLs also.

There are two ways we can get the Mega menu test to successfully run in only changed apps builds:

Recap

The only Cypress platform test that should run for only changed app builds is the Mega menu test (src/platform/site-wide/mega-menu/tests/megaMenu.cypress.spec.js). The other platform Cypress tests shouldn't be run for the reasons mentioned above. In situations where files outside of apps on the allow list are modified, all platform Cypress tests can be run, which is the current behavior.

timwright12 commented 2 years ago

@theodur @alexandec I think this looks good, I think we should, rather than skipping tests, find them a new home. E.g. recommending moving the facilities sidebar test into static pages, etc.

meganhkelley commented 2 years ago

Thank you for the very thorough write up! I'm going to loop in @JoeTice @pjhill to this conversation — for review when y'all have a chance.

I'm reading this as: in a worst case, "omg everything has gone wrong" scenario, the only thing a change to an app could affect outside of that app is the megamenu.

Is that correct? Or, is there any other website functionality that could be affected by an app change that we need to be testing for?

Edit: in case anyone else happens upon this, by "change to an app" I specifically mean a react app in vets-website that has been isolated enough to be added to the allowlist for isolated builds.

theodur commented 2 years ago

@timwright12 Initially it seemed like moving some of these tests to other directories would make sense, but looking into the ones we talked about, they are all with their respective code. For example:

Facilities Sidebar test: This test is located in the src/platform/site-wide/side-nav directory, which makes sense since the code for the side nav lives in that directory. Moving this test would mean moving the entire side-nav directory into the static-pages app, which is a bigger lift, but there could be an argument to be made there.

Header search functionality test: This one is located in the src/platform/site-wide/user-nav directory, which contains components for the user nav, like the search menu (SearchMenu.jsx). Moving this test would also mean moving that component, which probably wouldn't be best since the search menu is part of the user nav in the header.

User navigation test: Despite using the Dashboard app for testing, this test is testing the user nav explicitly, which can be done with any app page and the home page, so it wouldn't belong anywhere else.

theodur commented 2 years ago

@meganhkelley That's correct, I don't see any site wide functionality being affected by apps that will be on the allow-list, (other than static-pages, which isn't going to be on the list). If anyone can recall a time when only changes to an app affected site wide functionality, it would be good to reference that here, or even include instructions on how to replicate app changes that would cause this.

pjhill commented 2 years ago

I'm going to quote reply and respond to the recommendation to be sure that I don't miss any points.

Recommendation

Tests that shouldn't be run

The forms system tests shouldn't need to be run for only changed apps builds. The chances of changes to app code affecting the forms system is slim to none. This was an issue when the forms system imported a few form definitions from other apps, but those definitions have been moved out of the apps and into the forms system in the cross app import work.

This makes logical sense to me, and that PR is great. The unauthenticated user test also probably doesn't need to be run in only changed apps builds. The authentication functionality that apps use is separate from the apps themselves, therefore there shouldn't be any reason why changes to an app's code would affect authentication at a site-wide level. Because this test checks unauthenticated functionality using a few arbitrary apps, one alternative would be to dynamically use the changed app in CI to run this test on. The only caveat in that is that not all apps in the repo require authentication, so if an only changed apps build in CI was for an app like the coronavirus-screener, this test would fail.

I'm not aware of any entanglement between specific apps and the common authentication mechanisms for the platform. It should be safe to remove. Perhaps it would be prudent for Testing Tools Team to make some updates to the Cypress best practices documentation in order to recommend that VFS teams include authenticated and unauthenticated flows in their E2E test specs if it is relevant for their product. In the site wide platform tests, there are many tests that probably don't need to be run:

  • Facilities Sidebar test: src/platform/site-wide/side-nav/tests/e2e/sideNav.cypress.spec.js

    • This solely tests the functionality of the facilities sidebar, which is a React widget that needs the static-pages app built in order to render. The only app that should be able to affect the sidebar's functionality is the static-pages app, and that app will not be placed on the changed apps build allow-list for various reasons, so we can assume it's safe to not run this test.

Solid reasoning.

  • Smoke test: src/platform/site-wide/tests/sanity-check/smoke-test.cypress.spec.js

    • This test shouldn't need to be run with only changed apps builds, because it's mostly a sanity check to ensure that the body of the home page for the website loads. There shouldn't be any reason why the home page for the site shouldn't load due to changes in an app. App assets aren't loaded on the homepage, with the exception of the static-pages app, but that app won't be placed on the allow-list.

Yep, not a huge fan of this as an "E2E" test anyway.

  • Header search functionality test: src/platform/site-wide/user-nav/tests/e2e/00-required.cypress.spec.js

    • This test is for the search functionality in the site header. This functionality is mostly related to the search app via the SearchMenu component. Since the functionality is for the most part isolated to the search app and SearchMenu component, changes to other apps shouldn't impact the functionality.

Yeah, agreed this functionality is defined by the search app and should be tested only in that context.

  • User navigation test: src/platform/site-wide/user-nav/tests/user-nav.cypress.spec.js

    • Although this test uses the dashboard app to test with, it is testing the header navigation of a logged-in user, so this could be done with any app, and even the homepage. Because this tests the functionality of an authenticated user, this probably doesn't need to be run since other apps shouldn't affect the functionality of the header when a user is logged in. It is possible that an app could break the header in general, but that case would most likely be covered in the Mega menu test, which should be run in only changed apps builds.

Yeah, having thought this through your reasoning makes sense to me. We could potentially expand the scope of the megaMenu test to be sure as well.

Tests that should be run

The test that probably should be run in only changed apps builds is the Mega menu test: src/platform/site-wide/mega-menu/tests/megaMenu.cypress.spec.js. This is an overall test of the header that ensures it is being rendered properly with the correct menus. This would be useful to test to ensure that app changes won't interfere with the header in any unexpected ways. This test should also be enough to cover the situations mentioned above with other site-wide tests, because app changes shouldn't affect their functionality, and if app changes did, it would most likely be because of issues with rendering the header, which this test should cover.

Agreed, we should ensure that app changes don't adversely affect the menus. What's an example of a scenario where an app could interfere with proper functioning or rendering of the megaMenu on the homepage?

Currently, the Mega menu test is run on the homepage. This works fine when the static-pages app is built, since that's the only app asset that's loaded on non-app pages, like the homepage. When the static-pages app isn't built, this test will fail, because the homepage doesn't have any application asset to load the header from. The header is injected into app assets from the startApp function, which is why every app is able to load the header on their page. This means that the mega menu test can be run on app URLs also.

Oh, I see, I'll leave my above comment because it helps me to reason out my thinking.

There are two ways we can get the Mega menu test to successfully run in only changed apps builds:

    1. Building the static-pages app for all only changed apps builds (Not ideal since we don't want to build other apps than the ones that were changed)
    1. Updating the Mega menu test to dynamically use the URL of a modified app when only changed apps are built in CI. (This could be a bit hacky, as we would need to get the entry name of a changed app, so we can use the get the rootUrl from its manifest and pass that into the .visit() commands in the test.)

I think I like option 2. I like the idea of using the manifest file to derive the appropriate URL.

Recap

The only Cypress platform test that should run for only changed app builds is the Mega menu test (src/platform/site-wide/mega-menu/tests/megaMenu.cypress.spec.js). The other platform Cypress tests shouldn't be run for the reasons mentioned above. In situations where files outside of apps on the allow list are modified, all platform Cypress tests can be run, which is the current behavior.

Cool -- do you want us to work on redesigning the megaMenu test spec or are you going to take a pass?

CBonade commented 2 years ago

Having touched most, if not all of these tests during our conversion from Nightwatch, all recommendations related to what needs to run/not run on a single app build sound good to me.

I had a few thoughts similar to Tim above regarding if we couldn't handle some of these tests differently as well whether it's finding them a new home, or having a standardized, dummy component that uses the same design system elements/functionality to test against that lives in the platform level, instead of relying on an app that may or may not exist (can apps someday become irrelevant and be removed?). Just thoughts that passed through my mind while reading this. Totally separate from the task at hand here.

theodur commented 2 years ago

@pjhill Thanks for taking a look at this!

I'm not aware of any entanglement between specific apps and the common authentication mechanisms for the platform. It should be safe to remove. Perhaps it would be prudent for Testing Tools Team to make some updates to the Cypress best practices documentation in order to recommend that VFS teams include authenticated and unauthenticated flows in their E2E test specs if it is relevant for their product.

I think adding documentation on this is a good idea. There are some apps that test unauthenticated flows, but not a lot of them do. An additional way of encouraging unauthenticated flow tests could be to add an optional setting to the Cypress form tester that would run the unauthenticated flow test on the app's URL before running the test on the form. Since that test is pretty simple, it shouldn't be too bad to add it to the form tester as an optional test to run. This way, all developers would have to do is turn it on.

Cool -- do you want us to work on redesigning the megaMenu test spec or are you going to take a pass?

It'd be great if Testing Tools had any suggestions on how to go about making the Mega menu test dynamic. One of the ways I had in mind was somehow passing the app's entry name as an environment variable to the test, and having the test extract the root URL from the manifest that way. It'd be great to see if you all had any other ideas though.

theodur commented 2 years ago

@CBonade Those are good points. The cases where I feel like that's especially an issue is in the forms system tests. Almost all of those tests are using the HCA app for it's testing, which is problematic for many reasons. It would be nice to have those tests use an example form.

I feel like the side nav itself could probably be moved to the static pages app since it appears on static pages. And that test would just move with it.

Some of the functionality in the user nav test (00-required.cypress.spec.js) could probably be extracted to the search app, and that test could be refactored to test at a more general level like you mentioned.

I think beyond the scope of this, having an audit of platform directory as a whole could be useful in determining how tests in there should be structured, and the best place where things should live.

theodur commented 2 years ago

Closing this since the implementation has been merged ✅