-
Notifications
You must be signed in to change notification settings - Fork 230
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
Refactor article cypress tests to same format as NextJS Cypress E2Es #12364
Conversation
/** | ||
* TODO: Determine whether when running scheduled E2Es, should we run the smoke URLs too? | ||
* Or should we only run the non-smoke tests - and leave the smoke tests for the Simorgh CD pipeline | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a second opinion on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're thinking of smoke tests as things to catch big fails early on as they just test critical functionality, i'd be tempted to just run them as part of the simorgh CD pipeline i.e. only when changes are made? As then they'd be a thing that determines if we're happy to go ahead with further testing as part of the scheduled E2E's. Fair few definitions of smoke testing online seem to define it as just preliminary testing , which i think fits more in the CD pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good shout. The smoke tests will have already run on the pipeline (and would have given us confidence that things are OK), so we don't need to re-run them again on the scheduled E2Es. I am happy with this rubber-🦆-ing so I can go and make the changes now 😄
crossPlatformTests, | ||
]; | ||
|
||
const smokeCanonicalTestSuites = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lottttttt of test URLs - are they really necessary?
{ smoke: true, pageType: 'articles', environment: 'local' }
[
'/gahuza/articles/c5y51yxeg53o',
'/mundo/articles/ce42wzqr2mko',
'/news/articles/cn7k01xp8kxo',
'/persian/articles/cej3lzd5e0go',
'/pidgin/articles/cwl08rd38l6o',
'/scotland/articles/czwj5l0n210o',
'/serbian/articles/c805k05kr73o/cyr',
'/serbian/articles/c805k05kr73o/lat',
'/zhongwen/articles/c3xd4x9prgyo/simp',
'/zhongwen/articles/c3xd4x9prgyo/trad'
]
{ smoke: true, pageType: 'articles', environment: 'test' }
[
'/mundo/articles/ce42wzqr2mko',
'/news/articles/cn7k01xp8kxo',
'/persian/articles/cej3lzd5e0go',
'/pidgin/articles/cwl08rd38l6o',
'/pidgin/articles/crrrkxz2k0ko'
]
{ smoke: true, pageType: 'articles', environment: 'live' }
[
'/mundo/articles/ce7p1pw7165o',
'/news/articles/cj7xrxz0e8zo',
'/persian/articles/crgxnrdl1xvo',
'/persian/articles/cld9872jgyjo',
'/pidgin/articles/cgwk9w4zlg8o',
'/pidgin/articles/cw8qv1d11l9o'
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any documentation/PRs that explain the choices of these urls? It does seem a lot, would it be okay to just be testing one LTR, one RTL, and variant services as that covers i think most cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, no documentation at all. Many of the URLs are pretty old, and have been around since we began testing. I believe that when we first started with Optimo, we created articles on test and live, and we tend to use those in our tests. e.g. https://www.test.bbc.com/pidgin/articles/cwl08rd38l6o and https://www.bbc.com/pidgin/articles/cgwk9w4zlg8o (most, if not all services have one of these). However, they are a bit outdated, and do not fully showcase all article capabilities.
I imagine as time went on, and we added new functionality to the Article page, we tried to find an article which was relevant to that new behaviour, and we just chucked it on the list.
We could use this as an opportunity to 'document' (via code comments?) why we're including this particular asset. e.g. persian because RTL. gahuza because lite etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, not gonna block the merging of the PR over this as realtively minor overall ! But i suppose for future if we wanted to speed our tests up we could look to cut these down a bit and document somewhere why we have chosen whatever urls we end up with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I was hoping this could be something that @MeriemMechri and @paruchurisilpa could look into in future. For the purposes of this PR, it was to provide a template/example for how to migrate the cypress tests over to format we use in NextJS (and the benefit is having all the URLs in one place, instead of scattered around in the settings file). Making the tests run faster is something that can be done later.
…o article-cypress-tests-v1
… removed from the cypress file
…o article-cypress-tests-v1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - Ran all tests mentioned above in the testing section and they are all passing ✅ I will follow the same pattern for other page types.
@emilysaffron would you be happy for approve this, so that I can get it merged in? |
Resolves JIRA [number]
Overall changes
Implement the same format used in the NextJS Cypress tests: https://github.com/bbc/simorgh/blob/be8446b6f3a6cb20c5eb01a02c98bdb6ca2a47ad/ws-nextjs-app/cypress/e2e
Code changes
Testing
CYPRESS_APP_ENV=test yarn cypress:interactive
CYPRESS_APP_ENV=live yarn cypress:interactive
CYPRESS_SMOKE=false CYPRESS_APP_ENV=test yarn cypress:interactive
CYPRESS_SMOKE=false CYPRESS_APP_ENV=live yarn cypress:interactive
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines