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

Test framework with github actions #3689

Merged
merged 15 commits into from
Oct 2, 2020

Conversation

michalsn
Copy link
Member

Description
This PR is an attempt to replace TravisCI with Github Actions.

Checklist:

  • Securely signed commits

@michalsn
Copy link
Member Author

Well... the drop in coverage is interesting. I have no idea what went wrong 😅

@samsonasik
Copy link
Member

I think coverage decreased is fine as far as the the test still works.

@paulbalandan
Copy link
Member

My guess for the decreased coverage is that coveralls only considered the lines covered by the first run using SQLite3. Runs for MySQLi and Postgres were not considered anymore. Is it even possible to consolidate the runs?

@michalsn
Copy link
Member Author

michalsn commented Sep 28, 2020

Thanks, I'm testing this right now - will see :)

Oh.. .and about consolidation run... I have no idea - I don't see such an option anywhere.

@paulbalandan
Copy link
Member

I think we need to set up COVERALLS_PARALLEL: true for multiple runs. Then setup another job for coveralls_finish.

https://github.com/twinh/php-coveralls#github-actions

app/Config/Database.php Outdated Show resolved Hide resolved
@michalsn
Copy link
Member Author

@paulbalandan Oh... it was there and I missed it - thanks :)

@MGatner
Copy link
Member

MGatner commented Sep 29, 2020

The new DB Config content looks good! Probably need to rebase.

@michalsn
Copy link
Member Author

Great, I broke something and I didn't even notice it. Will try to fix it.

@michalsn michalsn requested a review from MGatner September 29, 2020 16:12
@samsonasik
Copy link
Member

The badge build in readme.md need to be updated to github action badge

@MGatner
Copy link
Member

MGatner commented Sep 30, 2020

Could we name the workflow "PHPUnit" to be consistent with our other test workflows?

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Looking very good! A few things I should have mentioned earlier. But I also want @lonnieezell to approve the removal of TravisCI before we merge, since that is a pretty big organizational decision. We could always run tests on both in the interim, but that has disadvantages.

tests/_support/Config/Registrar.php Outdated Show resolved Hide resolved
.github/workflows/test-phpunit.yml Outdated Show resolved Hide resolved
@lonnieezell
Copy link
Member

I'm all for doing this. Travis has become really slow, whereas Actions seems to be much faster, and having everything in one place is great.

Is there a way to do an "allowed failure" and test against 8 yet with Actions? I know it will fail currently, but it's something we need to get in place sometime soon.

@MGatner
Copy link
Member

MGatner commented Sep 30, 2020

@lonnieezell There's a lot of noise about "allowed failures" but basically it looks like there is not currently an equivalent feature to the way Travis lets us test against nightlies. Check out this issue if you want to contribute to said noise: actions/runner#2347

@MGatner
Copy link
Member

MGatner commented Sep 30, 2020

For now, since the plan is to release 4.1 alongside the PHP changes maybe we should add PHP 8 to the matrix for 4.* branches?

@lonnieezell
Copy link
Member

@MGatner That's too bad. The continue on failure action doesn't seem to help here, either, I don't think. Oh well.

But yes, I think adding 8 to 4.1 would be good.

@MGatner
Copy link
Member

MGatner commented Sep 30, 2020

Oooo the experimental flag is a nice find! I'm excited to see how this turns out - I might be adding it to my modules to start getting them ready for PHP 8.

@michalsn
Copy link
Member Author

michalsn commented Sep 30, 2020

Well... idk if this will work out as we want it to but fingers crossed.
I'm still working with continue-on-error here.

@michalsn
Copy link
Member Author

Ok... this is not working. I guess we would have to make all checks successful 😕 Probably it's possible but have no much sense.

@michalsn
Copy link
Member Author

michalsn commented Oct 1, 2020

Ok, this is a bit redundant but I have no other ideas on how to handle this. Right now PHP 8.0 has its own job which is only run for 4.1 branch.

Too bad that matrix is not available for the if condition on the "main" level. It would simplify our file a lot.

If someone has any other ideas on how to handle this, I'm all ears. Hopefully, github will introduce something similar to allow-failure soon.

@MGatner
Copy link
Member

MGatner commented Oct 1, 2020

Thanks for your work on this! Your experience is very valuable for the big upcoming November stuff.

I think for now I'd rather see a different YML file pushed to the 4.1 branch and develop not deal with 8 at all. IMO 8 is not worth testing right now in any definitive way, and maybe GitHub will have some better solution available by November. 🤞

@michalsn
Copy link
Member Author

michalsn commented Oct 1, 2020

Yeah... I'm totally fine with excluding PHP8. We can address this later in 4.1 branch.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Agreed. Looks great! Travis is taking 2 hours for a run - excited to see if this speeds is up!

@michalsn michalsn merged commit 1be8896 into codeigniter4:develop Oct 2, 2020
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.

6 participants