wagtail-nest / wagtail-bakery

A set of helpers for baking your Django Wagtail site out as flat files.
MIT License
179 stars 40 forks source link

Update test suite and move to Github Actions #62

Closed Stormheg closed 3 years ago

Stormheg commented 3 years ago

This pull request does a few things :sweat_smile:

1. Update tox envlist

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 https://github.com/wagtail/wagtail/pull/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.

Stormheg commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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

loicteixeira commented 3 years ago

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:

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 commented 3 years ago

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!

Stormheg commented 3 years ago

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

Stormheg commented 3 years ago

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

loicteixeira commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.