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

Run the tests automatically via Github Actions Workflow #469

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

dbast
Copy link
Contributor

@dbast dbast commented Sep 3, 2023

This adds a GitHub Action workflow to run the automated tests via pytest. GitHub Action workflows are freely available for OpenSource Projects and can be specified via yaml files in the repo.

The PR does the most simple thing by setting up Python, installing the dependencies and running the tests bare metal on the ubuntu-latest x86_64 runners, which makes it pretty fast.

Automatic test runs could make PR reviews easier to some extend :)

Here a link to the actions run in the fork of the repo: https://github.com/dbast/seedsigner/actions/runs/6063280014/job/16450424545

TODO: This requires somebody with permissions approving the Action run for this PR (due to new contribution) or maybe go to the Actions tab / settings of the repo and enable/allow actions.

@jdlcdl
Copy link

jdlcdl commented Sep 3, 2023

I really like!

Some thoughts to consider:

  • If these runs are saved historically, I like the pytest -v switch which is still not "too-verbose", and gives hints as to which tests existed at a particular moment in time (even though we have a habit of filling-out/adding related tests within the same test functions).
  • We also have ./tests/screenshot_generator/generator.py which I use in two ways. 1) to know how many screenshots were generated and "Do they ALL have a current modified-timestamp?", else the ones that don't failed (a hint that something unintended was affected). 2) as sort-of a ux-designer's "acceptance test suite" that I, not a designer, enjoy over coffee. Note: there is a known warning that would be nice to track-down/remove before this becomes part of an automated run on github.
  • we also have coverage, which gives us a "quantitative-only" measure of improvement towards test-coverage. If we knew the coverage after each pr, we could watch this 'needle' move, assuming we can also see the previous coverage report.

None of my comments above are to be interpreted as a NACK. In the spirit of "FOSS is a slow, sometimes dreadfully slow, march towards better." I fully support this as "better" and future pr's could continue that march.

ACK!!!!

@dbast
Copy link
Contributor Author

dbast commented Sep 3, 2023

@jdlcdl Added coverage reporting + more pytest options (like -vv, durations of the slowest 5 tests). The coverage is printed to the test log output, but also a html report is generated and uploaded.

You can see a full run here https://github.com/dbast/seedsigner/actions/runs/6065958918 ... click on the job to see the log or on the ci-artifacts to download the html report.

I am also happy to add running ./tests/screenshot_generator/generator.py and also storing that as downloadable artifact. Tried it and it fails not finding seedsigner-icons.ttf. Guess I have to do a more extensive font setup to get the generator.py files run correctly. Any hints?

@jdlcdl
Copy link

jdlcdl commented Sep 3, 2023

Tried it and it fails not finding seedsigner-icons.ttf. Guess I have to do a more extensive font setup to get the generator.py files run correctly. Any hints?

When setting up an RPi for development work, I usually follow the "Manual Installation". You'll notice much of this is geared particularly for raspberry-pi, but since the last release 0.6.0, much has been separated so that a non-RPi development computer can run the unit tests and the screenshot generator. It's possible that documentation is not complete and/or screenshot_generator imports are not sufficient, since this capability on non-RPi is recent. Perhaps follow that same document while ignoring RPi steps. I see that you're installing both requirements.txt files and not the requirements-raspi.txt, also seedsigner itself into the environment; this is good. The sudo apt imports are fewer than I expected, and I don't understand the open-sans fonts install. All fonts necessary for seedsigner should be in ./src/seedsigner/resources/fonts/ in the repo... and the two which I'm familiar with are .otf files.

@dbast
Copy link
Contributor Author

dbast commented Sep 4, 2023

@jdlcdl Figured it out:

  • Fonts are searched relative to the code in components.py with font_path = os.path.join(pathlib.Path(__file__).parent.resolve(), "..", "resources", "fonts")
  • The fonts are only relative to the code after installing with python setup.py install/pip install . by including them via MANIFEST.in as package data and also setting include_package_data=True in setup.py
  • Allowed me to also remove the fonts-open-sans install command, which did not make sense with this context

So this PR now:

  1. runs the tests (red / green test status for every new PR after this is merged)
  2. print the coverage to the pytest log and also produces a html coverage report
  3. generates the screenshots
  4. uploads html coverage report + screenshots

A full run with the downloadable ci-artifacts can be currently found in my fork, see https://github.com/dbast/seedsigner/actions/runs/6070632443 ... yay, all the screenshots are there :)

@kdmukai
Copy link
Contributor

kdmukai commented Sep 4, 2023

@dbast wow, that's amazing progress! Very cool.

Now I'm getting greedy:

  • Is it possible to make the test artifacts browsable rather than just downloadable?
  • The ultimate wishlist would be to somehow show a screenshot diff (i.e. which ones changed) vs the screenshot repo. Not expecting that to be practical, though.

fyi: After v0.7.0 is released, @newtonick and I have a bit of downtime planned (~2-3wks for me) to catch up on other things, provided there are no urgent bugs to attend to.

But this PR is definitely an important addition and hopefully we can give it more attention when we're back.

@dbast
Copy link
Contributor Author

dbast commented Sep 4, 2023

@kdmukai While there is no direct possibility to make the artifacts browsable, there are other things that could help:

  • the workflow could be enabled to post PR comments (to the PR it got triggered from) including the screenshots OR post the screenshots to an image hoster liker https://imgur.com/ and add direct links as comment to the PR ... both options have some advantages and disadvantages
  • the seedsigner-screenshots repo could be additionally cloned in the workflow and then:
    • a visual diff can be calculated (via some tool like imagemagick)
    • newly generated pictures can be shown next to the existing ones
    • either made accessible via PR comment ... or via MD file as part of the downloadbale ci-artifacts.zip file.

Would do this in a separate PR as the goal of this PR is to get the tests runnings in a fully automated way with as little as possible changes to the existing code base.

@jdlcdl
Copy link

jdlcdl commented Sep 4, 2023

I believe that there are already reasonable diffs of my cloned seedsigner_screenshots repo, as long as I generate them , commit and push them alongside code changes. Images are so pixel-for-pixel correct that the differences are reserved to where an intended code change actually altered a screen, exactly what we want, with an exception for some screens where the screenshot generator is randomizing inputs ie: seed backup words. In a perfect run, all pngs get recreated with a fresh timestamp, but only ones that actually changed are seen as 'modified' by git.

@newtonick
Copy link
Collaborator

ACK

@newtonick newtonick merged commit f7ec9ff into SeedSigner:dev Sep 14, 2023
@dbast dbast deleted the ci branch September 14, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged Not Yet Released
Development

Successfully merging this pull request may close these issues.

4 participants