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.

Sign up for GitHub

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

Jump to bottom

Feat(PPDSC-2315): implement Applitools spike recommendations #324

Merged
merged 13 commits into from
Aug 9, 2022
Merged

Feat(PPDSC-2315): implement Applitools spike recommendations #324

merged 13 commits into from
Aug 9, 2022

Conversation

mstuartf
Copy link
Contributor

@mstuartf mstuartf commented Aug 3, 2022

PPDSC-2315

What

  1. Background - implementation of Applitools spike recommendations
  2. What did you do
  • Add comments in code / docs explaining the importance of the Automated batch closing feature of the Applitools GitHub integration.
  • Simplify Applitools config (shared config in env variables, suite-specific config in config files, no config in package.json scripts).
  • Add component and docs site tests to the same batch.
  • Add custom property suite to distinguish comp and doc tests.
  1. What does the reviewers should expect
  • Comp and doc site tests should be in the same batch with suite prop to distinguish them.
  • Should not need to re-run job after resolving a test failure.

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

mstuartf added 5 commits August 3, 2022 09:25
@mstuartf
feat(PPDSC-2315): save all env variables to bash
c7a7b63 
- Env variables need to be added to $BASH_ENV to be accessed across steps.
- "In every step, CircleCI uses bash to source BASH_ENV. This means that BASH_ENV is automatically loaded and run, allowing you to use interpolation and share environment variables across run steps." - https://circleci.com/docs/env-vars
@mstuartf
feat(PPDSC-2315): simplify applitools config
c0aee1b
@mstuartf
feat(PPDSC-2315): comments to explain config
d9dd03e
@mstuartf
feat(PPDSC-2315): add comp and doc tests to same batch
37aeceb 
- Don't close batches after SDK finishes.
- Manually close batch after doc site tests.
- Prevent test jobs failing when test suite fails.
@mstuartf
feat(PPDSC-2315): custom props to filter test suites in ui
c4075a6
@mstuartf mstuartf added the WIP Work in progress label Aug 3, 2022
@github-actions github-actions bot added the feature This change contains a new feature label Aug 3, 2022
@newskit-engineering
Copy link
Collaborator

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2315-applitools-config
Storybook: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2315-applitools-config/storybook

@mstuartf mstuartf force-pushed the feat/PPDSC-2315-applitools-config branch from d2a1afe to b7e32fa Compare August 3, 2022 11:31
@mstuartf mstuartf added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Aug 3, 2022
@mstuartf mstuartf force-pushed the feat/PPDSC-2315-applitools-config branch 2 times, most recently from 9afa980 to fefbc41 Compare August 5, 2022 08:00
@mstuartf mstuartf added WIP Work in progress and removed ready for review Please assist in getting this reviewed labels Aug 8, 2022
@mstuartf mstuartf force-pushed the feat/PPDSC-2315-applitools-config branch from cf677fb to ac84c30 Compare August 8, 2022 08:48
@mstuartf mstuartf added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Aug 8, 2022
@Vanals
Copy link
Contributor

Vanals commented Aug 9, 2022

To optimize the money saved I guess we are not merging the two jobs in one? for comp and docs?

@mstuartf
Copy link
Contributor Author

mstuartf commented Aug 9, 2022

To optimize the money saved I guess we are not merging the two jobs in one? for comp and docs?

Yeah I think it still makes sense to keep them separate. If you only make changes to the doc site there's no need to run component tests.

@Vanals
Copy link
Contributor

Vanals commented Aug 9, 2022

To optimize the money saved I guess we are not merging the two jobs in one? for comp and docs?

Yeah I think it still makes sense to keep them separate. If you only make changes to the doc site there's no need to run component tests.

Yep, would not make sense for that one as is just one test. I would when you have to run two proably

@mstuartf mstuartf merged commit 3ef2907 into main Aug 9, 2022
@mstuartf mstuartf deleted the feat/PPDSC-2315-applitools-config branch August 9, 2022 14:30
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
@mstuartf
Feat(PPDSC-2315): implement Applitools spike recommendations (#324)
e3b15fc 
* feat(PPDSC-2315): save all env variables to bash

- Env variables need to be added to $BASH_ENV to be accessed across steps.
- "In every step, CircleCI uses bash to source BASH_ENV. This means that BASH_ENV is automatically loaded and run, allowing you to use interpolation and share environment variables across run steps." - https://circleci.com/docs/env-vars

* feat(PPDSC-2315): simplify applitools config

* feat(PPDSC-2315): comments to explain config

* feat(PPDSC-2315): add comp and doc tests to same batch

- Don't close batches after SDK finishes.
- Manually close batch after doc site tests.
- Prevent test jobs failing when test suite fails.

* feat(PPDSC-2315): custom props to filter test suites in ui

* feat(PPDSC-2315): correct custom props syntax

* feat(PPDSC-2315): add step to skip applitools tests

* feat(PPDSC-2315): remove i have merged applitools check

* feat(PPDSC-2315): use sdk to close doc site batch

* feat(PPDSC-2315): fix curl syntax

* feat(PPDSC-2315): smaller image

* feat(PPDSC-2315): comment to explain why jobs won't fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mutebg mutebg mutebg approved these changes

@Vanals Vanals Vanals approved these changes

@jps jps Awaiting requested review from jps

@Xin00163 Xin00163 Awaiting requested review from Xin00163

@pdimova pdimova Awaiting requested review from pdimova

@baburay23 baburay23 Awaiting requested review from baburay23

@evgenitsn evgenitsn Awaiting requested review from evgenitsn

@Srinivasan-Ramasamy Srinivasan-Ramasamy Awaiting requested review from Srinivasan-Ramasamy

Assignees
No one assigned
Labels
feature This change contains a new feature ready for review Please assist in getting this reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mstuartf @newskit-engineering @Vanals @mutebg