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.

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

ci(PPDSC-2017): newskit integration test #183

Merged
merged 26 commits into from
May 10, 2022

Conversation

Vanals
Copy link
Contributor

@Vanals Vanals commented Apr 29, 2022

PPDSC-2017

What

  • Created new CircleCi job test_newskit_in_nextjs_app.
    It Installs needed packages (yalc, create-next-app, etc.)
    Creates the NextJs app
    Prepares the NExt App adding files(for cy) and overriding the index.js, also installs other packages.
    Once the app is ready, through start-server-and-test serves the app and tests it using cy.

  • Created folder in scripts containing few template files for this app.

  • Also replicated the issue we released, importing the scenario from the wrong file. The app crashed and the test failed.
    it failed even before cy opening the page. start-server-and-test does check already for 200 before testing.

@jps
Cy might actually not be needed, BUT is a nice setting and nice to have if we foresee adding CY integration tests in the future. If not start-server-and-test will be all we need and we can remove CY reducing the job time by around 1 min.

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:

@Vanals Vanals added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels May 3, 2022
@Vanals Vanals added the feature This change contains a new feature label May 3, 2022
echo "Installing needed packages into the NextJS app"

cd newskit-integration-test
yalc add newskit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It seems that with yalc add my-package, we don't need yarn link anymore. I assume any local changes in newskit will be reflected in this react app automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was having some issues when using yarn link. facebook/react#14257.
Only yalc helped me. Not sure if at the end worked cos react needed to be downgraded. Anyway, overall, yalc seem to be better thank yarn link!

Xin00163
Xin00163 previously approved these changes May 5, 2022
Makefile Outdated Show resolved Hide resolved
Co-authored-by: Evgeni Nikolov <evgeni.nikolov@news.co.uk>
mutebg
mutebg previously approved these changes May 9, 2022
Copy link
Contributor

@mutebg mutebg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super cool, this is very much how I imagined as well 👏

@mutebg mutebg changed the title Ci/ppdsc 2017 newskit integration test ci(PPDSC-2017): newskit integration test May 9, 2022
evgenitsn
evgenitsn previously approved these changes May 9, 2022
jps
jps previously approved these changes May 9, 2022
Copy link
Contributor

@jps jps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

scripts/templates-integration-test/index.txt Outdated Show resolved Hide resolved
Co-authored-by: James Spencer <james.spencer@news.co.uk>
@Vanals Vanals dismissed stale reviews from jps, evgenitsn, and mutebg via b73524b May 10, 2022 08:00
@Vanals Vanals merged commit 605bead into main May 10, 2022
@Vanals Vanals deleted the ci/PPDSC-2017-newskit-integration-test branch May 10, 2022 13:15
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
* docs(PPDSC-0000): testing permissions

* docs(PPDSC-0000): removing test changed from readme

* ci(PPDSC-2017): created job and script for integr test

* ci(PPDSC-2017): added job in pr workflow

* ci(PPDSC-2017): removed yarn build and imrpoved requires

* ci(PPDSC-2017): improved comments in template, added back yarn build

* ci(PPDSC-2017): updated template

* ci(PPDSC-2017): removed index.js

* ci(PPDSC-2017): added -p flag in mkdir command

* ci(PPDSC-2017): adding caching and fixing last run

* ci(PPDSC-2017): fixed chaching and increased v

* ci(PPDSC-2017): changing executor and how install cy

* ci(PPDSC-2017): added note in MakeFile, updated Cy installation

* ci(PPDSC-2017): testing speed with no cache

* ci(PPDSC-2017): improved cy config and tests

* ci(PPDSC-2017): improve comment in makefile, yalc publish and spec fix

* ci(PPDSC-2017): adding test job in workflows

* ci(PPDSC-2017): improved naming step

* Update Makefile

Co-authored-by: Evgeni Nikolov <evgeni.nikolov@news.co.uk>

* Update scripts/templates-integration-test/index.txt

Co-authored-by: James Spencer <james.spencer@news.co.uk>

Co-authored-by: Evgeni Nikolov <evgeni.nikolov@news.co.uk>
Co-authored-by: James Spencer <james.spencer@news.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This change contains a new feature ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants