Setup End to End Tests #325

kylepotts · 2022-08-20T18:18:46Z

Should help start the ball following on solving #286 (comment)

commit db95c3f Author: Kyle Potts <[email protected]> Date: Sat Aug 20 13:19:06 2022 -0400 update readme commit 8733340 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 13:12:33 2022 -0400 trying a different reporter commit 15551b4 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 12:59:30 2022 -0400 add detailed summary commit 4973f92 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 12:53:50 2022 -0400 adding -v commit b84b17c Author: Kyle Potts <[email protected]> Date: Sat Aug 20 12:49:56 2022 -0400 no working directory commit 80bd97e Author: Kyle Potts <[email protected]> Date: Sat Aug 20 12:45:34 2022 -0400 upload test report commit 34abe1a Author: Kyle Potts <[email protected]> Date: Sat Aug 20 12:09:28 2022 -0400 set base url for e2e commit 55d1b12 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 11:12:31 2022 -0400 adding more tests commit 921bf69 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 08:29:50 2022 -0400 remove other install commit a38fba4 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 08:26:36 2022 -0400 use correct requirement.txt commit 0c16ca4 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 08:24:04 2022 -0400 update gitignore for pycache commit d7c5955 Author: Kyle Potts <[email protected]> Date: Sat Aug 20 08:23:39 2022 -0400 first stab at adding playwright

…ting.com into feat/e2e-tests

elreydetoda · 2022-08-20T22:14:41Z

I'd like to review this one before it's merged 🥳

Thank you so much @kylepotts for working on this! I'm super excited to take a look 🥳 🚀

StefanS-O · 2022-08-21T19:12:20Z

For me it looks good, it has it's own Github Action so it won't interfere with the build. I am not sure about storing screenshots as artifacts, because i don't know limits to that.

I have never worked with playwright, only cypress. I wonder how we could test more complicated pages with more JavaScript interaction, like the single episode page with the audio player. Looking forward to exploring that.

I think it is a good starting point for exploring further, thanks @kylepotts 🚀

StefanS-O · 2022-08-21T19:13:21Z

Let's see what @elreydetoda says!

gerbrent · 2022-08-22T20:22:09Z

nice contribution @kylepotts ! Waiting for additional feedback from our resident experts....

elreydetoda

Overall, very nicely done @kylepotts! 🥳 🎉

Everything seems to make logical sense, and I ran things locally to validate everything was passing like expected (which it was). Thank you so much for this contribution I know I personally really appreciate it and I'm sure the JB staff does too! 😁

Now for the review part...I know that I've left a lot of comments (not all of it is actionable, some of it's acknowledgement/praise) and I've opened 3 PRs against your repo. So, I've got a bit of feedback 😅 , but it shouldn't be too hard because the PRs should get you most of the way. Definitely feel free though to re-implement it yourself, for learning purposes, if you want to but I wanted to make sure the repo has those changes for the initial addition of the E2E testing.

Most of the feedback is smaller things and some best practices I've learned from videos/classes I've taken about pytest. Pretty much everything from a playwright perspective (beside the one pieces of feedback about the pause) looked really solid 😁

Also, something that pylint has complained about me using before is that instead of .format() for formatting string, it normally recommends using f-strings instead.

Let me know if you have any questions about any of the feedback, and it might also be beneficial if you want to run black & pylint against your files as well. It makes it more cohesive, from a formatting perspective, when we're all using/linting our stuff the same way.

BTW, how was your first experience using playwright?

elreydetoda · 2022-08-23T09:48:49Z

test/e2e/home.py

@@ -0,0 +1,128 @@
+import re


I don't think this is used anywhere, is it?

Suggested change

import re

elreydetoda · 2022-08-23T10:46:36Z

test/e2e/home.py

+
+def test_homepage_screenshot(page: Page):
+    page.goto("/")
+    page.pause()


Typically (from my understanding) it's better to use something like .wait_for_load_state(). That'll ensure the page has fully loaded and there is no more network traffic (i.e. loading)

Suggested change

page.pause()

page.wait_for_load_state('networkidle')

Actually I think this can probably go. I was just using it for debugging.

elreydetoda · 2022-08-23T11:41:45Z

.github/workflows/e2e.yml

+      - name: Set up Python
+        uses: actions/setup-python@v4
+        with:
+          python-version: '3.10'
+      - name: Install dependencies
+        working-directory: ./test
+        run: |
+          python -m pip install --upgrade pip
+          pip install -r requirements.txt
+      - name: Ensure browsers are installed
+        run: python -m playwright install --with-deps


I've already submitted a PR against your repo to help with the transition I'm suggesting, but I'd prefer to use pipenv instead of just raw pip + pip freeze for deps mgmt. Just how we did for the scraper (look at that issue for justifications as well).

Then transition to using pipenv like how we did for scrape.yml:

jupiterbroadcasting.com/.github/workflows/scrape.yml

Lines 27 to 40 in b519f05

- name: 'Setup Python'

uses: actions/setup-python@v3

with:

python-version: '3.10'

architecture: 'x64'

cache: "pipenv"

- name: 'Install Python deps'

run: pip install pipenv && cd ./show-scraper && pipenv sync --bare

- name: 'Scrape'

run: |

cd ./show-scraper

DATA_DIR=../jbsite LATEST_ONLY=true pipenv run ./scraper.py

elreydetoda · 2022-08-23T11:45:17Z

.github/workflows/e2e.yml

+      - name: run hugo build
+        uses: jakejarvis/hugo-build-action@master 
+        with:
+          args: --gc --config ./config.toml -b http://localhost:1313
+      - name: run server
+        run: |
+          cd public && python -m http.server 1313 &


I don't know how I feel about this...for now I think this is fine, but eventually I think we should be targeting the branches output. For example, if it's a feature branch and we've got the deploy previews configured, then point the base URL to that feature branch's deploy preview or with production actually point to to the production URL.

Then we should be able to remove the hugo build step here completely, and just ensure this comes after the deployment. I think that'll be simpler in the long run, but for now this should work.

@kylepotts, do you mind making an issue about this so we don't forget about it 😁

But that means that these tests don't get run on each commit which sort of defeats the purpose of having tests doesn't it? Only you plan on deploying each branch someone makes to an environment to test and then tear it down.

So, yes and no 🙃

My plan for deploy previews was a tiered approach.

Have them build on every pull request (not on JB infrastructure, but on netlify) (outlined here)

Have them run on merge into develop

Have them run on merge into prod

So, in that situation they would run on every push of commits, because netlify re-builds + GH actions re-run when commits are added to an already open PR.

(BTW, just to be on the same page I'm talking strictly about a future implementation)

elreydetoda · 2022-08-23T11:58:11Z

.github/workflows/e2e.yml

+      - name: Publish Test Report
+        uses: dorny/test-reporter@v1
+        if: always()
+        with:
+          name: E2E Tests
+          path: './test/report.xml'
+          reporter: java-junit


I really like this nice html summary artifact (preview here) that gets added to the action 😁

elreydetoda · 2022-08-23T11:59:56Z

.github/workflows/e2e.yml

+      - name: Save screenshots
+        uses: actions/upload-artifact@v2
+        with:
+          name: screenshots
+          path: test/screenshots/


This will be awesome for a quick check of the webpages 😁

Eventually we could also add video recordings too, but lets not add too much to this PR yet (probably need to chat with the JB team about storage first).

elreydetoda

After this convo we'll just approve this for now and I'll submit these changes later.

Just waiting on @ironicbadger's approval now.

… best practices

Feat/test improvments

elreydetoda

Awesome, it all looks good to me 😁

Thanks again 🥳

gerbrent · 2022-08-24T18:25:25Z

Here's my hesitation on this PR, which is worthy of much more discussion and is more a meta-topic for the project than anything and not at all a reflection of the commits/code:

While I am positive this is probably an amazing addition to our project, and really fulfills one of the main asks for the new website - i.e. "help us w your expertise to make an amazing thing together!" - it seems we are now beginning to push outward the skills of those who are in place to understand the whole project, from a longevity point of view. Speaking personally, I am just about zero help in assessing the impacts of this PR as it surpasses my skill-set. But I see, aswell, that others who are here to check on the features and inner-workings of the new website are beginning to not comprehend the entirety of what's being integrated.

@elreydetoda has clearly been an amazing asset here, so kudos Elrey!

We need to keep thinking about the 5 year datapoint: those involved in JB at a deep level in 5 years - which will necessarily be different from today - will still need to be using and understanding everything we put into this project, or at least have a roadmap of documentation/comments/explainers to help rediscover it's parts when they break (they will).

I think what I'm asking is: Can we get an explainer/documentation of what's happening here for those of us who will likely need to maintain these parts for years? I would LOVE to understand, even on a surface level, which aspects this is solving and on simple terms, how it is solving it.

Also: ❤️ for all the hard and creative work!

gerbrent · 2022-08-24T18:45:05Z

Here's what I gather, from your helpful tests/Readme.md, and poking around:

This sets up Playwright, a helpful testing automation framework, that will allow us to use browser engines to mimic actual user interactions, and optionally capture screenshots/screencasts of said tests. This will allow us to make sure commits/PRs do not inadvertently break unrelated features by standardizing known-true states of our working site.

test/conftest.py suggests expected items to look for, and presumably report on if broken. I also assume other tests can/will be written and evolved.., even for commit/branch-specific uses.

How am I doing?! ; )

gerbrent · 2022-08-24T18:52:04Z

test/Readme.md

+
+    `pytest --base-url http://localhost:1313 e2e/*`
+
+4. Add your own test to the e2e folder and get coding!


I would imagine it would be helpful to mention the newly added tests command from the Makefile?

Also, previously we have been using docker to standardize local testing deployments, could this also be applied here in the Playwright testing environment to automate and thus avoid local dependency issues?

The Makefile uses docker, so why not the local testing environment aswell as described in the Readme?

The Readme was made before we updated to the Dockerfile for tests, so that needs to be updated.

test/Readme.md

kylepotts · 2022-08-25T21:51:01Z

@gerbrent

The goal of of testing is to make it so your code changes and deploys are inherently less risky. By nature of the website being a software project that is going to be actively worked on we need things in place to make sure that we do not break the website with our changes.

If you wanted to, you could deploy and manually spend time testing the site to see if you broke something. You might catch it you might not. But that manual process is painful and not very effective.

In the world of software, there has been a lot of time and attention placed into automating these style of manual tests make sure that as we change things were not breaking them as go.

These tests are setup to run a headless browser and verify that certain elements on the page exist. They check if the header logo is still there and if the header contains the correct links in the dropdowns. It also takes a screenshot of the page so someone can manually eyeball it for review to make sure nothing is out of place.

As far as your comment on the 5 year mark. These areas of software engineering are constantly evolving. There is always some new shiny testing framework. In 5 years I can guarantee these frameworks will likely be very different then they are now. Tests are not meant to be left in the dust and forgotten though. They are meant to be maintained just as you maintain the code you write.

In the end, this was my attempt at trying to make it so there is less risk in making changes to this project that will cause problems with the jbsite when its deployed. The tests are supposed to increase the confidence that code changes will not cause issues when deployed to production. Any good software project will have tests it is in my opinion a needed and necessary part of any software project if you expect it to be maintained and worked on.

If you decided this type of testing is not valuable because no one will know how to maintain it then that is the projects choice. I would still advocate for some kind of testing especially as you all are considering automatically deploying these changes from main into the production jb site.

update testing readme

kylepotts · 2022-08-25T22:12:59Z

@gerbrent

I've updated the testing readme. Let me know if you would like more details on it.

…ting.com into feat/e2e-tests

gerbrent · 2022-08-26T00:29:14Z

@kylepotts thanks for all that!

I think the functionality is a wonderful addition to this project, just want to make sure it's something that can be used by many of us, and modified by many of us aswell. Sounds like that might not be a far stretch, so I fully support this idea given the technical bits can be confirmed/checked, which feels like @StefanS-O and @elreydetoda already confirmed.

Once you're happy w your merge cleanups, let me know @kylepotts and I'll happily merge this at a time when a few of us can be available to troubleshoot it's integration, if needed.

@kylepotts if you'd like to be involved during that merge session, we'd happily have you.

test/Readme.md

… intention.

gerbrent · 2022-08-26T01:00:01Z

(and someone please tell me if I'm making a bigger deal out of this merge than is needed 😄 )

gerbrent

Great Readme!

gerbrent · 2022-08-26T02:08:42Z

This seems fun:

Test Generator

Playwright comes with the ability to generate tests out of the box and is a great way to quickly get started with testing. It will open two windows, a browser window where you interact with the website you wish to test and the Playwright Inspector window where you can record your tests, copy the tests, clear your tests as well as change the language of your tests.

Run codegen and perform actions in the browser. Playwright will generate the code for the user interactions. codegen will attempt to generate resilient text-based selectors.

https://playwright.dev/docs/codegen-intro

gerbrent · 2022-08-27T17:22:17Z

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

kylepotts added 6 commits August 20, 2022 08:23

first stab at adding playwright

d7c5955

update gitignore for pycache

0c16ca4

Setup e2e tests

2736d86

Merge branch 'feat/e2e-tests' of github.com:kylepotts/jupiterbroadcas…

65ee6a2

…ting.com into feat/e2e-tests

spelling is hard

906fce1

elreydetoda self-requested a review August 20, 2022 22:14

StefanS-O self-requested a review August 21, 2022 09:56

StefanS-O approved these changes Aug 21, 2022

View reviewed changes

gerbrent assigned ironicbadger Aug 22, 2022

elreydetoda mentioned this pull request Aug 23, 2022

Using pipenv for dependency mgmt kylepotts/jupiterbroadcasting.com#3

Closed

elreydetoda requested changes Aug 23, 2022

View reviewed changes

elreydetoda approved these changes Aug 23, 2022

View reviewed changes

kylepotts added 5 commits August 23, 2022 19:24

add dockerfile for tests, add Makefile for tests, and file for pytets…

e35110c

… best practices

Merge pull request #6 from kylepotts/feat/test-improvments

37660e1

Feat/test improvments

remove junky commits

9f843ce

use pytest ini file

44fe484

Merge pull request #7 from kylepotts/feat/test-improvments

14bc997

Feat/test improvments

elreydetoda added the enhancement New feature, enhancement, or request label Aug 23, 2022

elreydetoda approved these changes Aug 24, 2022

View reviewed changes

This was referenced Aug 24, 2022

Adding some pytest best practices kylepotts/jupiterbroadcasting.com#5

Closed

Adding Makefile target for tests kylepotts/jupiterbroadcasting.com#4

Closed

gerbrent requested a review from ironicbadger August 24, 2022 18:07

gerbrent reviewed Aug 24, 2022

View reviewed changes

gerbrent added 2 commits August 24, 2022 22:37

Update test/Readme.md: typo

8bd1d18

Update test/Readme.md: typo

1484362

kylepotts added 2 commits August 25, 2022 18:09

update testing readme

f395ca9

Merge pull request #8 from kylepotts/feat/test-improvments

1c5f94a

update testing readme

kylepotts added 8 commits August 25, 2022 18:14

update testing readme

5d01daa

remove files

f129268

Merge branch 'feat/e2e_tests' into feat/e2e-tests

c2a161f

Merge branch 'feat/e2e-tests' of github.com:kylepotts/jupiterbroadcas…

f35bfb2

…ting.com into feat/e2e-tests

update readme

33945c3

I really messed up that merge

029b2d1

update readme again

ecd5bc8

still fixing bad merge

b058e8f

StefanS-O approved these changes Aug 26, 2022

View reviewed changes

gerbrent approved these changes Aug 26, 2022

View reviewed changes

Apply suggestions from code review - spelling, typos, grammar, links,…

153639f

… intention.

gerbrent reviewed Aug 26, 2022

View reviewed changes

StefanS-O merged commit b7c30ad into JupiterBroadcasting:main Aug 27, 2022

This was referenced Aug 29, 2022

E2E GH Action - patch/modifying #353

Merged

E2E - update Live menu item check address #370

Merged

elreydetoda linked an issue Sep 11, 2022 that may be closed by this pull request

Deployment smoke tests #286

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup End to End Tests #325

Setup End to End Tests #325

kylepotts commented Aug 20, 2022

elreydetoda commented Aug 20, 2022

StefanS-O commented Aug 21, 2022 •

edited

Loading

StefanS-O commented Aug 21, 2022

gerbrent commented Aug 22, 2022

elreydetoda left a comment •

edited

Loading

elreydetoda Aug 23, 2022

elreydetoda Aug 23, 2022

kylepotts Aug 23, 2022

elreydetoda Aug 23, 2022

elreydetoda Aug 23, 2022

kylepotts Aug 23, 2022

elreydetoda Aug 23, 2022 •

edited

Loading

elreydetoda Aug 23, 2022

elreydetoda Aug 23, 2022

elreydetoda left a comment

elreydetoda left a comment

gerbrent commented Aug 24, 2022 •

edited

Loading

gerbrent commented Aug 24, 2022 •

edited

Loading

gerbrent Aug 24, 2022

gerbrent Aug 24, 2022

kylepotts Aug 25, 2022

kylepotts commented Aug 25, 2022 •

edited

Loading

kylepotts commented Aug 25, 2022

gerbrent commented Aug 26, 2022

gerbrent commented Aug 26, 2022

gerbrent left a comment

gerbrent commented Aug 26, 2022

gerbrent commented Aug 27, 2022 •

edited

Loading

	- name: 'Setup Python'
	uses: actions/setup-python@v3
	with:
	python-version: '3.10'
	architecture: 'x64'
	cache: "pipenv"

	- name: 'Install Python deps'
	run: pip install pipenv && cd ./show-scraper && pipenv sync --bare

	- name: 'Scrape'
	run: \|
	cd ./show-scraper
	DATA_DIR=../jbsite LATEST_ONLY=true pipenv run ./scraper.py


		`pytest --base-url http://localhost:1313 e2e/*`

		4. Add your own test to the e2e folder and get coding!

Setup End to End Tests #325

Setup End to End Tests #325

Conversation

kylepotts commented Aug 20, 2022

elreydetoda commented Aug 20, 2022

StefanS-O commented Aug 21, 2022 • edited Loading

StefanS-O commented Aug 21, 2022

gerbrent commented Aug 22, 2022

elreydetoda left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elreydetoda Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elreydetoda left a comment

Choose a reason for hiding this comment

elreydetoda left a comment

Choose a reason for hiding this comment

gerbrent commented Aug 24, 2022 • edited Loading

gerbrent commented Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylepotts commented Aug 25, 2022 • edited Loading

kylepotts commented Aug 25, 2022

gerbrent commented Aug 26, 2022

gerbrent commented Aug 26, 2022

gerbrent left a comment

Choose a reason for hiding this comment

gerbrent commented Aug 26, 2022

gerbrent commented Aug 27, 2022 • edited Loading

StefanS-O commented Aug 21, 2022 •

edited

Loading

elreydetoda left a comment •

edited

Loading

elreydetoda Aug 23, 2022 •

edited

Loading

gerbrent commented Aug 24, 2022 •

edited

Loading

gerbrent commented Aug 24, 2022 •

edited

Loading

kylepotts commented Aug 25, 2022 •

edited

Loading

gerbrent commented Aug 27, 2022 •

edited

Loading