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

Dockerize System Tests #710

Merged
merged 2 commits into from
Oct 28, 2023
Merged

Conversation

lcreid
Copy link
Contributor

@lcreid lcreid commented Oct 21, 2023

@donv very nicely added system tests that generate screenshots for the README. This PR adds support to the optional Docker and Docker Compose files so that the system tests can be run in Docker.

The intent is to ensure that system tests can still be run without Docker, but that they will also run for those who prefer to develop in a Docker environment, because they want to isolate their development work, or for whatever other reason.

This PR introduces a new Docker Compose file: docker-compose-system-test.yml. This file uses a standard Selenium image for Chrome to run the browser. Part of the reason for a separate file is the current docker-compose.yml results in smaller containers, since it doesn't have the full browser. I also wanted to try things in a completely separate file, to avoid breaking the existing Docker workflow.

The approach in this PR is highly experimental and includes some rough edges:

  • Actually running the tests is manual. There are a few steps to it, and while they may be familiar to people used to working with Docker, they're still inconvenient.
  • As documented, files generated by the tests are owned by root. There may be a simple fix for this, but I haven't tried it yet.
  • I haven't confirmed that system tests still work for non-Docker users.
  • The screenshots generated by the Docker workflow are one pixel bigger on the x-axis than the ones stored in the repository.
  • The locale needs to be set in the Selenium container to be consistent with the existing screenshots.

@lcreid lcreid self-assigned this Oct 21, 2023
@lcreid lcreid requested a review from donv October 21, 2023 22:38
@lcreid
Copy link
Contributor Author

lcreid commented Oct 21, 2023

@donv would you mind pulling this branch and testing if it still runs system tests however you were running them before?

@lcreid
Copy link
Contributor Author

lcreid commented Oct 23, 2023

I think it may be a dream to make the system tests work on all possible platforms people could be using, since none of them have exactly the same fonts. At least, I haven't seen a good solution to this problem. Let me know if you've seen screenshot tests that work across platforms.

@donv
Copy link
Collaborator

donv commented Oct 23, 2023

@lcreid I got a problem:

Minitest::UnexpectedError: LoadError: cannot load such file -- sassc

Will investigate.

@lcreid
Copy link
Contributor Author

lcreid commented Oct 23, 2023

I should have mentioned that I have been using Rails 7.0.8 while working on this PR.

@lcreid
Copy link
Contributor Author

lcreid commented Oct 26, 2023

@donv Did you generate the screenshots for the README on a Mac, or Windows, or Linux (distro?)? The reason I ask is that I'm trying to find a font on Linux that will be close (or the same) metrics-wise to your screenshots, so we can at least visually scan the screenshots quickly for significant differences.

@donv
Copy link
Collaborator

donv commented Oct 27, 2023

@donv Did you generate the screenshots for the README on a Mac, or Windows, or Linux (distro?)? The reason I ask is that I'm trying to find a font on Linux that will be close (or the same) metrics-wise to your screenshots, so we can at least visually scan the screenshots quickly for significant differences.

I generated the screenshots on MAC.

I have recently, in another project, implemented a scheme for generating the screenshots in GitHub actions and updating them using pull requests from a separate branch for screenshots. This seems to work very well, and I think that should be the way forward.

Copy link
Collaborator

@donv donv left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and all tests are green in my workspace using your fork+branch.

@lcreid
Copy link
Contributor Author

lcreid commented Oct 28, 2023

I have recently, in another project, implemented a scheme for generating the screenshots in GitHub actions and updating them using pull requests from a separate branch for screenshots. This seems to work very well, and I think that should be the way forward.

Brilliant! I've created an issue for this, and assigned it to myself. But I'll probably un-assign myself in a day or two, when I don't find time to work on it. Feel free to assign yourself.

@lcreid lcreid marked this pull request as ready for review October 28, 2023 00:15
@lcreid lcreid merged commit b750707 into bootstrap-ruby:main Oct 28, 2023
11 checks passed
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.

None yet

2 participants