test(WESTEAMA-1180): update topicPage tests to not use BFF #11554

MeriemMechri · 2024-04-23T16:28:03Z

Overall changes

Refactor and stabilise tests for topic page.

Code changes

Removed cy.getPageData function and replaced it with getPageDataFromWindow
Used window.SIMORGH_DATA.pageData for canonical tests
Unskipped test
Added a BeforeEach() function, to ensure we always start from the path being tested and make the tests deterministic

Testing

Cypress Tests on PR passing

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

cypress/e2e/pages/topicPage/tests.js

LilyL0u · 2024-04-25T17:12:58Z

cypress/e2e/pages/topicPage/tests.js

    describe(`Pagination`, () => {
      it('should show pagination if there is more than one page', () => {
-        // First return to the topics page. Last test has page on article
-        cy.go('back');
+        if (pageCount === undefined) {


Do we need this conditional here to skip the test?

If we do, do we want to have a log to say why the pageCount might be undefined? It could be undefined because we are on the wrong page, like how we saw when we looked at the test failures from being still on an article page. You visiting the pages in the BeforeEach in the describe block above should ensure this doesn't happen. Is there another reason why it could happen, and if so, should be log something so we don't start skipping the first test without knowing? (although, the following tests would fail anyway if the pagination isn't present).

I don't think we need this - we check below whether pageCount is > 1, and will only run the pagination assertions if that's the case. Let's remove this.

LilyL0u · 2024-04-25T17:16:26Z

cypress/e2e/pages/topicPage/tests.js

        const selector = '[data-testid="topic-promos"]:first > li';
        const promoCount = Cypress.$(selector).length;
        cy.log(`Number of promos on the page${promoCount}`);
-
        if (promoCount !== numberOfItems) {
          cy.window().then(win => {
            const pageData = win.SIMORGH_DATA;


If we want to keep this log (I think we should, especially as we are seeing if this helps flakiness), shall we use the data we got on line 35?

@LilyL0u I think we agreed that we no longer needed this log, because the number of items should always be consistent with the promoCount since the data is coming from window.SIMORGH_DATA and not potentially outdated / "too new" (i.e. data is newer than the page being tests) BFF data

But, we can leave it as is, and monitor it in Live for a while, and remove at a later date?

karinathomasbbc

Nice work @MeriemMechri, thanks for fixing this up, this will hopefully bring stability to these tests going forward!

cypress/e2e/pages/topicPage/tests.js

karinathomasbbc · 2024-04-26T09:02:31Z

cypress/e2e/pages/topicPage/tests.js

        const selector = '[data-testid="topic-promos"]:first > li';
        const promoCount = Cypress.$(selector).length;
        cy.log(`Number of promos on the page${promoCount}`);
-
        if (promoCount !== numberOfItems) {
          cy.window().then(win => {
            const pageData = win.SIMORGH_DATA;


@LilyL0u I think we agreed that we no longer needed this log, because the number of items should always be consistent with the promoCount since the data is coming from window.SIMORGH_DATA and not potentially outdated / "too new" (i.e. data is newer than the page being tests) BFF data

But, we can leave it as is, and monitor it in Live for a while, and remove at a later date?

karinathomasbbc · 2024-04-26T09:03:36Z

cypress/e2e/pages/topicPage/tests.js

    describe(`Pagination`, () => {
      it('should show pagination if there is more than one page', () => {
-        // First return to the topics page. Last test has page on article
-        cy.go('back');
+        if (pageCount === undefined) {


I don't think we need this - we check below whether pageCount is > 1, and will only run the pagination assertions if that's the case. Let's remove this.

karinathomasbbc · 2024-04-26T09:04:24Z

cypress/support/commands/application.js

+export const getPageDataFromWindow = () => {
+  cy.window().then(win => {
+    const pageData = win.SIMORGH_DATA;
+    return pageData;
+  });
+};

 Cypress.Commands.add('testResponseCodeAndType', testResponseCodeAndType);
 Cypress.Commands.add('testResponseCodeAndTypeRetry', testResponseCodeAndType);
 Cypress.Commands.add('getPageData', getPageData);
+Cypress.Commands.add('getPageDataFromWindow', getPageDataFromWindow);


Just a note that we might need to be careful of a conflict if Lily's article fetching PR is merged first (but we can figure that out depending on which PR is merged first)

LilyL0u · 2024-04-26T09:13:43Z

#11554 (comment)

I'm not sure about having it in a position where it runs for all the tests, as the pagination tests are a sequential user journey (e.g go to second page by clicking number 2, check page number, go to third page by clicking the next arrow, check page number. Next/Previous button tests won't have the same results if we go back to the same page before each test). For the page content tests this is fine, but not the pagination ones.

LilyL0u · 2024-05-08T14:28:25Z

cypress/e2e/pages/topicPage/tests.js

        // Checks number of items on page
        cy.get('[data-testid="topic-promos"]')
          .first()
          .children()
          .its('length')
          .should('eq', numberOfItems);
      });
-      it.skip('First item has correct headline', () => {
+
+      it('First item has correct headline', () => {


The reason I skipped this test originally was because sometimes the promo headline is different from the full article headline, which causes a failure, but it is expected as sometimes there is a shortHeadline which is different from the longer one used in the article.

I am thinking maybe we either remove this test or just leave it to see if this still happens. What do you think @karinathomasbbc ?

I think that's a good shout - add logging if it doesn't exist, and keep an eye on the channel so that we can quickly disable / fix if it becomes an issue?

Yes, that makes sense .. I already added a cy.log, let me know if that's enough to monitor the test

Ah yes - in fact, the test won't fail anymore because you're checking if firstItemHeadline has a value and then exiting, so the tests won't be "flaky" anymore, but there is no guarantee that it will always run the assertions.

test(WESTEAMA-1180): update topicPage tests to not use BFF refactor: fix failing test by adding goBack(() function and reomve any unecessary spaces/ console logs Refactor: removed console log refactor: fix failing test by adding BeforeEach function that will take us back each time to the current path that being tested fix: fix linting issues refactor: move to async/await instead of promise refactor(pr-feedback): remove unnecessary conditon

MeriemMechri changed the title ~~(DRAFT) test(WESTEAMA-1180)/ update topicPage tests to not use BFF~~ (DRAFT) WESTEAMA-1180 update topicPage tests to not use BFF Apr 23, 2024

MeriemMechri commented Apr 24, 2024

View reviewed changes

cypress/e2e/pages/topicPage/tests.js Show resolved Hide resolved

MeriemMechri force-pushed the tests/stabilise-topics-page branch 3 times, most recently from 0345d1c to 8aef79a Compare April 24, 2024 21:07

MeriemMechri commented Apr 24, 2024

View reviewed changes

cypress/e2e/pages/topicPage/tests.js Show resolved Hide resolved

MeriemMechri changed the title ~~(DRAFT) WESTEAMA-1180 update topicPage tests to not use BFF~~ WESTEAMA-1180 update topicPage tests to not use BFF Apr 24, 2024

MeriemMechri requested a review from karinathomasbbc April 25, 2024 08:12

LilyL0u reviewed Apr 25, 2024

View reviewed changes

karinathomasbbc requested changes Apr 26, 2024

View reviewed changes

karinathomasbbc approved these changes May 8, 2024

View reviewed changes

LilyL0u reviewed May 8, 2024

View reviewed changes

LilyL0u approved these changes May 9, 2024

View reviewed changes

MeriemMechri force-pushed the tests/stabilise-topics-page branch 4 times, most recently from c375d4d to 2907160 Compare May 12, 2024 17:19

MeriemMechri force-pushed the tests/stabilise-topics-page branch from 2907160 to 0b9393f Compare May 12, 2024 17:20

MeriemMechri changed the title ~~WESTEAMA-1180 update topicPage tests to not use BFF~~ WESTEAMA-1180: update topicPage tests to not use BFF May 12, 2024

MeriemMechri changed the title ~~WESTEAMA-1180: update topicPage tests to not use BFF~~ test(WESTEAMA-1180): update topicPage tests to not use BFF May 12, 2024

MeriemMechri merged commit e660e77 into latest May 13, 2024
11 checks passed

MeriemMechri deleted the tests/stabilise-topics-page branch May 13, 2024 07:57

karinathomasbbc mentioned this pull request May 13, 2024

Revert "test(WESTEAMA-1180): update topicPage tests to not use BFF" #11619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(WESTEAMA-1180): update topicPage tests to not use BFF #11554

test(WESTEAMA-1180): update topicPage tests to not use BFF #11554

MeriemMechri commented Apr 23, 2024 •

edited

LilyL0u Apr 25, 2024

karinathomasbbc Apr 26, 2024

LilyL0u Apr 25, 2024

karinathomasbbc Apr 26, 2024

karinathomasbbc left a comment

karinathomasbbc Apr 26, 2024

karinathomasbbc Apr 26, 2024

karinathomasbbc Apr 26, 2024

LilyL0u commented Apr 26, 2024

LilyL0u May 8, 2024 •

edited

karinathomasbbc May 8, 2024

MeriemMechri May 9, 2024

karinathomasbbc May 9, 2024

test(WESTEAMA-1180): update topicPage tests to not use BFF #11554

test(WESTEAMA-1180): update topicPage tests to not use BFF #11554

Conversation

MeriemMechri commented Apr 23, 2024 • edited

Overall changes

Code changes

Testing

Helpful Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karinathomasbbc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LilyL0u commented Apr 26, 2024

LilyL0u May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeriemMechri commented Apr 23, 2024 •

edited

LilyL0u May 8, 2024 •

edited