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

Screenshots with playwright #3023

Merged
merged 4 commits into from
Feb 3, 2022

Conversation

holtchesley
Copy link
Contributor

No description provided.

playwright/Dockerfile Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #3023 (e937ffb) into develop (c77929c) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3023      +/-   ##
===========================================
+ Coverage    84.46%   84.52%   +0.05%     
===========================================
  Files           50       50              
  Lines         5530     5530              
===========================================
+ Hits          4671     4674       +3     
+ Misses         859      856       -3     
Impacted Files Coverage Δ
perma_web/api/serializers.py 94.85% <0.00%> (+1.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c77929c...e937ffb. Read the comment docs.

@holtchesley
Copy link
Contributor Author

Some questions with this:

  • Do we go with Ubuntu or Debian for the base image?
  • Is having functional testing infra in a separate container still seeming like a viable path?
  • Are we still using Sauce Labs? Where is it being called from if we are?
  • Do we want to use a service like Save Labs/Browserstack/etc for testing IE,iPhone, multiple OSs... ?
  • The dev.screenshot task that we've got right now doesn't work for me(FireFox isn't installed in the container), is this worth fixing/documenting?

@holtchesley
Copy link
Contributor Author

holtchesley commented Jan 31, 2022

Do we go with Ubuntu or Debian for the base image?

The pro for Debian is that it's similar to our other images and that might simplify some maintenance. The upside of Ubuntu is that we get more reliable long term support with less fussing for FF+Webkit in the image. One of the advantages of playwright is that it's got compatible browsers bundled in, so I'm leaning towards Ubuntu.

@holtchesley
Copy link
Contributor Author

Is having functional testing infra in a separate container still seeming like a viable path?

We lose access to some of the nice internal-to-django stuff like url reversal and such, but perhaps that's a reasonable tradeoff.

@rebeccacremona
Copy link
Contributor

rebeccacremona commented Jan 31, 2022

Are we still using Sauce Labs? Where is it being called from if we are?

I have no memories of ever having seen this run via Sauce Labs; I think its use predates me. It was never run via CI: people initiated runs from their machine. It would be kind of interesting to find out if this setup works, but I could easily imagine Sauce Labs has a put out a new API or whatever in the interim. I think we can very reasonably ignore/remove any Sauce-related stuff here, and make a separate issue for re-setting it up, referencing the original code as necessary, if we want to get it going again.

@rebeccacremona
Copy link
Contributor

The dev.screenshot task that we've got right now

How about that! I didn't know this was around. Added in cb61ebb in order to produce the screenshots that were used for the docs at that point in time. Fun!

I think at that time, devs ran Perma on the metal of their machine. I don't remember Firefox being installed in the VirtualBox VM we moved to..... And, we did lots of experimenting and got extremely close to adding Firefox to our Docker container, once we moved from the VM to Docker, but I don't think we ever ended up merging that in.

So! I think this task hasn't worked in years. I wonder if, if working, the selectors it targets are still in the markup, if those screenshots are still in the docs... and if they are, if they presently up to date. This sounds like a very low priority side quest, maybe something fun for me to look at on a Friday.

If we were re-implementing today, might be able to use headless Chrome's --screenshot flag: Chrome is installed already for making captures.

@holtchesley
Copy link
Contributor Author

The dev.screenshot task that we've got right now

How about that! I didn't know this was around. Added in cb61ebb in order to produce the screenshots that were used for the docs at that point in time. Fun!

So! I think this task hasn't worked in years. I wonder if, if working, the selectors it targets are still in the markup, if those screenshots are still in the docs... and if they are, if they presently up to date. This sounds like a very low priority side quest, maybe something fun for me to look at on a Friday.

The ~equivalent task in the playwright fab file works now (as far as I can tell), so the selectors are all there.

@rebeccacremona
Copy link
Contributor

Do we want to use a service like Save Labs/Browserstack/etc for testing IE,iPhone, multiple OSs... ?

If it's affordable, would kind of love it for testing, say, the playback of one public Perma Link and one private one. It seems like overkill for the majority of our frontend code, but playback seems intricate enough that concrete assurance it works broadly might be appreciated.

What are you thinking, worth pulling the thread, or likely to be more work (either resurrecting the Sauce Labs code, or switching to Browserstack, etc.) than it's worth?

@rebeccacremona
Copy link
Contributor

Is having functional testing infra in a separate container still seeming like a viable path?

We lose access to some of the nice internal-to-django stuff like url reversal and such, but perhaps that's a reasonable tradeoff.

How's it feeling to you? Are you having doubts? I am kind of delighted any time we can get stuff out of the main container, so that the main container gets closer and closer to what's actually running in prod. I am remembering, for instance, this one very fun bug where I pushed code that unknowingly relied on a package that was installed incidentally as a dependency for a test library. Worked locally, tests passed including in CI.... boom, busted in real life lol.

Especially if we are able to take advantage of the pre-built playwright images, complete with installed browsers (which, as said, I'll believe when I see lol), this still sounds like a good plan to me.

Re: not having access to Django niceties like URL reversal, we could potentially set up something clever so that the main Django-running container passes the info the functional tests need. For instance: we could give the main perma container the ability to run docker commands (https://github.com/harvard-lil/perma-capture/blob/new-arch-with-vue/docker-compose.yml#L43), and then have pytest fire off each functional tests, sending along whatever info was needed as args to the invocation, or even using a volume to share files, or whatever. Might be more baroque than we need, but might be kind of slick.

@holtchesley holtchesley force-pushed the playwright-tests branch 2 times, most recently from 42cc5e3 to 9dfaf85 Compare February 1, 2022 14:47
@holtchesley
Copy link
Contributor Author

A quick summary of changes:

  • Got rid of the Debian Dockerfile
  • Added a functional test to verify the homepage loads, and one that captures example.com and verifies that it can playback regularly.
  • Updated the Nginx container to make the functional test work.
  • Functional tests can now be run with docker-compose exec playwright pipenv run pytest with som additional params outlined in https://playwright.dev/python/docs/test-runners

@holtchesley holtchesley force-pushed the playwright-tests branch 5 times, most recently from 2619d26 to 6869fb4 Compare February 2, 2022 20:30
@holtchesley holtchesley marked this pull request as ready for review February 2, 2022 20:48
@holtchesley
Copy link
Contributor Author

Since the last update:

  • Created Cross browser testing #3026 for a follow up on cross browser/os testing with an external service.
  • CircleCI runs the tests.
  • Perhaps more improved fixtures for any future testing we want to do.

I think this is probably ready to send out unless there's an issue with the nginx config change I made, or anything else in the PR.

Copy link
Contributor

@rebeccacremona rebeccacremona left a comment

Choose a reason for hiding this comment

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

Jazzed about this. I haven't seen playwright before; everything seems so clean and tidy!

I had a couple of minuscule questions about the Dockerfile and Pipfile, but LGTM!

playwright/Pipfile Outdated Show resolved Hide resolved
playwright/Dockerfile Outdated Show resolved Hide resolved
playwright/Dockerfile Outdated Show resolved Hide resolved
playwright/fabfile.py Show resolved Hide resolved
playwright/tests/test_functional.py Show resolved Hide resolved
services/docker/webrecorder/nginx/nginx.conf Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@holtchesley
Copy link
Contributor Author

holtchesley commented Feb 2, 2022

I believe you can just use ARG BUILDARCH here, which will give you access to a correct `${BUILDARCH} later in the file.

This fails for me locally unless I also supply an appropriate --build-arg (which, for the record, is something I'm happy to do if it makes it easier for other build stuff to work).

@rebeccacremona
Copy link
Contributor

I believe you can just use ARG BUILDARCH here, which will give you access to a correct `${BUILDARCH} later in the file.

This fails for me locally unless I also supply an appropriate --build-arg (which, for the record, is something I'm happy to do if it makes it easier for other build stuff to work).

Huh, funny, I see that https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope says these args are ":only available when using the BuildKit backend." I wonder if you have a different backend.

🤷 tbd

@holtchesley
Copy link
Contributor Author

I tested the bare ARG BUILDARCH in the latest pull, and it looks like the current CircleCI setup also doesn't recognize it.

@rebeccacremona
Copy link
Contributor

I tested the bare ARG BUILDARCH in the latest pull, and it looks like the current CircleCI setup also doesn't recognize it.

How about that. That must be what's up with harvard-lil/h2o#1494 too.

@rebeccacremona rebeccacremona merged commit 2daab84 into harvard-lil:develop Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants