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

Update test suite and move to Github Actions #62

Merged
merged 9 commits into from
May 18, 2021

Conversation

Stormheg
Copy link
Member

This pull request does a few things 😅

1. Update tox envlist

  • Split envlist to only include supported Django and Python versions
  • Remove Wagtail 2.10 as it is no longer supported
  • Add Wagtail 2.13 as current latest stable
  • Update to test against Wagtail main with latest compatible Django and Python

2. Fix Django 3.2 tests

Include a SECRET_KEY in tests. This prevents Django 3.2 tests from failing due to missing secret key.

3. Add Github Actions workflow

I've added a Github Actions test workflow. This will replace Travis CI as they've scaled back resources on open-source builds. See also wagtail/wagtail#6519

The Github Actions workflow will only run for pull requests and pushes to the master/main branch. The workflow doesn't test every single Wagtail/Django/Python combinations. In that regard in retains the behaviour of the Travis CI job by only testing 1. All supported Django/Wagtail combinations with the latest supported Python version. 2. The latest supported Django/Wagtail combination for the remaining Python versions.

* Split envlist to only include supported Django and Python versions
* Remove Wagtail 2.10 as it is no longer supported
* Add Wagtail 2.13 as current latest stable
* Update to test against Wagtail main with latest compatible Django and
Python
Prevents Django 3.2 tests from failing due to missing secret key.
@Stormheg
Copy link
Member Author

You can see the action in action (pun intended) on my fork: https://github.com/Stormheg/wagtail-bakery/actions/runs/839957418

I've temporarily disabled coveralls on my fork. We shouldn't forget to set it up with Github Actions after merging.

@zerolab
Copy link
Contributor

zerolab commented May 13, 2021

Nice once Storm!

Worth replacing .circleci/nightly tests with a GH Action too. e.g. https://github.com/torchbox/wagtailmedia/blob/main/.github/workflows/nightly-tests.yml

See https://github.com/dfunckt/django-rules/blob/master/.github/workflows/ci.yml#L67-L88 for a coveralls example as well as how you could run the tests directly with tox (the action becomes quite simpler, and the config is mostly in tox.ini so fewer places to update)

@Stormheg
Copy link
Member Author

Worth replacing .circleci/nightly tests with a GH Action too. e.g. https://github.com/torchbox/wagtailmedia/blob/main/.github/workflows/nightly-tests.yml

I'm not necessarily convinced on replacing Circle CI on the basis that it works well and I don't see what benefit GH Actions has over CircleCI. @zerolab is there a case for replacing it?

See https://github.com/dfunckt/django-rules/blob/master/.github/workflows/ci.yml#L67-L88 for a coveralls example as well as how you could run the tests directly with tox (the action becomes quite simpler, and the config is mostly in tox.ini so fewer places to update)

That's an interesting approach! Here are some takeaways

  • We'd still be maintaining the tox environments in both tox.ini and in the action. With the benefit that the action would be slightly more condensed (and thus easier to maintain?).
  • Instead of spawning jobs for each tox environment we'd only spawn jobs for each Python version which executes the tox environments for that Python version in sequence. I guess this would give us slightly less overhead (setting up python, downloading & installing tox...) as we'd be using less jobs. That would be slightly more environment friendly 🌍 🥬

Curious as to what other people think. Is the above approach worth it?

Copy link
Collaborator

@loicteixeira loicteixeira left a comment

Choose a reason for hiding this comment

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

Thanks for doing that @Stormheg. Also, nice trick splitting the environments in tox.ini, it makes it much easier to reason.

tox.ini Outdated Show resolved Hide resolved
@loicteixeira
Copy link
Collaborator

loicteixeira commented May 14, 2021

Regarding moving the nightly build to GH Actions, I'm neither for nor against it. I agree that it's working fine at the moment, but if we want it all in one place, why not. If you are to move it over to GH Actions though, we will need to find someone to add the SLACK_WEBHOOK_URL secret to the GH Actions as do not have the permissions to do so.

As for running tests directly with tox, I agree it would simplify the action and reduce overhead, but we also lose separate reports. I think we might want to at least keep Wagtail version separate? So we would have the following groups:

  • Latest Python / All Django / Wagtail LTS
    • py38-django{22,30,31}-wagtail211
  • Latest Python / All Django / Wagtail Current
    • py39-django{22,30,31,32}-wagtail213
  • Python X / Latest Django / Wagtail Current
    • py36-django32-wagtail213
    • py37-django32-wagtail213
    • py38-django32-wagtail213
  • Latest Python / Latest Django / Wagtail Main
    • py39-django32-wagtailmain
  • Linting
    • flake8,isort

So down to 7 builds instead of 13? But to be honest, I wonder how useful it is to test against all the Python versions for a third party module. If we were to drop this requirement, we would be down to 4 builds environments.

@Stormheg
Copy link
Member Author

But to be honest, I wonder how useful it is to test against all the Python versions for a third party module. If we were to drop this requirement, we would be down to 4 builds environments.

I think we should keep testing against older Python versions to make sure we don't introduce changes that are incompatible with older, but still supported Python versions.

Reducing to 7 builds sounds good to me!

In order to limit the amount of different combinations we will only test
* Wagtail LTS + all Django combinations on the latest supported Python.
* Wagtail current + all Django combinations on the latest supported
Python.
* Wagtail current + latest Django on remaining supported Python
versions.
* Keep testing Wagtail main branch using latest Python and Django.
@Stormheg
Copy link
Member Author

Refactored the action to reduce the amount of jobs. See run here: https://github.com/Stormheg/wagtail-bakery/runs/2583020136

.github/workflows/test.yml Outdated Show resolved Hide resolved
Co-authored-by: Dan Braghis <dan@zerolab.org>
@Stormheg
Copy link
Member Author

Actually, it looks like coveralls is currently not working at all: https://coveralls.io/github/wagtail/wagtail-bakery?branch=master

@loicteixeira
Copy link
Collaborator

Yeah I don't results were uploaded to coveralls.io before. I enabled the project on their dashboard but idk if it would work from your fork? We might have to merge to see the results.

@loicteixeira
Copy link
Collaborator

If there are no further comments, I'll merge this. We will see then if it fixes coveralls.io. Thanks for the PR @Stormheg.

@loicteixeira loicteixeira merged commit 3874475 into wagtail-nest:master May 18, 2021
@loicteixeira
Copy link
Collaborator

loicteixeira commented May 18, 2021

Well, that did not go as planned https://github.com/wagtail/wagtail-bakery/actions/runs/852364233

Edit: I removed the report to coveralls for now so the build is passing on the main branch and we can take the time to investigate.

@Stormheg
Copy link
Member Author

@loicteixeira thanks for merging! I'm not sure why coveralls is failing. My guess would be that there is no coverage information to upload for the isort and flake8 jobs.

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