search

bbc / simorgh

The BBC's Open Source Web Application. Contributions welcome! Used on some of our biggest websites, e.g.
https://www.bbc.com/thai
Other
1.38k stars 215 forks source link

WSTEAMA-1182 Stabilise topic tags page cypress tests by removing BFF fetch #11678

Closed MeriemMechri closed 2 months ago

MeriemMechri commented 2 months ago

Resolves JIRA [WSTEAMA-1182]

Overall changes

Refactor and stabilise tests for topic tags page.

Code changes

Testing

Environment Smoke Command Status
local true CYPRESS_APP_ENV=local CYPRESS_SMOKE=true yarn test:e2e N/A
local false CYPRESS_APP_ENV=local yarn test:e2e N/A
test true CYPRESS_APP_ENV=test CYPRESS_SMOKE=true yarn cypress Pass
test false CYPRESS_APP_ENV=test yarn cypress Pass
live true CYPRESS_APP_ENV=live CYPRESS_SMOKE=true yarn cypress Pass
live false CYPRESS_APP_ENV=live yarn cypress Pass

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

LilyL0u commented 2 months ago

Now that this test checks for the existence of related topics before running any assertions, I wonder if we can do some more cleanup of the test code surrounding the topic tags:

I think we should remove this test completely, and if needed, add /news/technology-60561162 to the cypress settings file under the news / storyPage config

https://github.com/bbc/simorgh/blob/853cf26191a10e1391b2b75d6bcbe361ac3fbe25/cypress/e2e/pages/storyPage/tests.js#L86-L94

Here, we should get rid of the if/else, and always run the checks - even though it will really only be applicable to article and story pages...

https://github.com/bbc/simorgh/blob/853cf26191a10e1391b2b75d6bcbe361ac3fbe25/cypress/e2e/pages/testsForAllPages.js#L19-L28

If we remove this test, I think we now don't have a test that will fail if topic tags stop showing on the page. So if topic tags stop rendering, we won't know because all the tests will just not run as they only run when topic tags are on the page. Maybe we could move the test though so it isn't just on a storyPage, but runs on two assets - one for story one for article?

Do all articles and story pages have topic tags? If so, instead of running this on all pages, we could just run on story and article pages and do not need to check for the existence of the tags to run the test. Or make sure all the test assets do have topic tags if possible.

karinathomasbbc commented 2 months ago

If we remove this test, I think we now don't have a test that will fail if topic tags stop showing on the page. So if topic tags stop rendering, we won't know because all the tests will just not run as they only run when topic tags are on the page. Maybe we could move the test though so it isn't just on a storyPage, but runs on two assets - one for story one for article?

Do all articles and story pages have topic tags? If so, instead of running this on all pages, we could just run on story and article pages and do not need to check for the existence of the tags to run the test. Or make sure all the test assets do have topic tags if possible.

If we feel like we need to have a test which would fail if the tags failed to render, then we could add a test to the canonical test suite (for all page types), which could check the data and make sure the tags are rendered? The difficulty here was that the same test was reused for canonical & amp, and we have no way of getting the data for amp without going to the BFF (which is what we wanted to avoid). @MeriemMechri let's work together on this.

karinathomasbbc commented 2 months ago

Now that this test checks for the existence of related topics before running any assertions, I wonder if we can do some more cleanup of the test code surrounding the topic tags: I think we should remove this test completely, and if needed, add /news/technology-60561162 to the cypress settings file under the news / storyPage config https://github.com/bbc/simorgh/blob/853cf26191a10e1391b2b75d6bcbe361ac3fbe25/cypress/e2e/pages/storyPage/tests.js#L86-L94

Here, we should get rid of the if/else, and always run the checks - even though it will really only be applicable to article and story pages... https://github.com/bbc/simorgh/blob/853cf26191a10e1391b2b75d6bcbe361ac3fbe25/cypress/e2e/pages/testsForAllPages.js#L19-L28

If we remove this test, I think we now don't have a test that will fail if topic tags stop showing on the page. So if topic tags stop rendering, we won't know because all the tests will just not run as they only run when topic tags are on the page. Maybe we could move the test though so it isn't just on a storyPage, but runs on two assets - one for story one for article?

Do all articles and story pages have topic tags? If so, instead of running this on all pages, we could just run on story and article pages and do not need to check for the existence of the tags to run the test. Or make sure all the test assets do have topic tags if possible.

The other alternative @LilyL0u is to add topic tag tests to integration tests - we can then either use a known page with tags, or make it data driven (and will only be using fixture data anyway). This would give us an early indication (i.e. fail fast on PRs) that there was an issue with topic tags not rendering.

LilyL0u commented 2 months ago

Now that this test checks for the existence of related topics before running any assertions, I wonder if we can do some more cleanup of the test code surrounding the topic tags: I think we should remove this test completely, and if needed, add /news/technology-60561162 to the cypress settings file under the news / storyPage config https://github.com/bbc/simorgh/blob/853cf26191a10e1391b2b75d6bcbe361ac3fbe25/cypress/e2e/pages/storyPage/tests.js#L86-L94

Here, we should get rid of the if/else, and always run the checks - even though it will really only be applicable to article and story pages... https://github.com/bbc/simorgh/blob/853cf26191a10e1391b2b75d6bcbe361ac3fbe25/cypress/e2e/pages/testsForAllPages.js#L19-L28

If we remove this test, I think we now don't have a test that will fail if topic tags stop showing on the page. So if topic tags stop rendering, we won't know because all the tests will just not run as they only run when topic tags are on the page. Maybe we could move the test though so it isn't just on a storyPage, but runs on two assets - one for story one for article? Do all articles and story pages have topic tags? If so, instead of running this on all pages, we could just run on story and article pages and do not need to check for the existence of the tags to run the test. Or make sure all the test assets do have topic tags if possible.

The other alternative @LilyL0u is to add topic tag tests to integration tests - we can then either use a known page with tags, or make it data driven (and will only be using fixture data anyway). This would give us an early indication (i.e. fail fast on PRs) that there was an issue with topic tags not rendering.

What we could do is keep the tests Meriem has put in testsForAllPages that check 'bodyElement.find(aside[aria-labelledby*='related-topics'] a).length > 0' before running the tests, and remove the test as suggested by Karina above (that runs the test on the specific asset '/news/technology-60561162.amp?renderer_env=live', and just replace the purpose of that removed test with an integration test. This means we keep the work done that covers many pages, and fill the gap of checking that topic tags still render with one integration test with fixture data.

As discussed in daily sync, this also keeps down scope creep from entering the territory of adding loads of integration tests in this work instead of an integration test specific PR: we only need to add one simple integration test that checks the topic tags render when expected in specific fixture data.