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

Add minimal phpunit run in GitHub Actions #44

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 11, 2021

Adds minimal PHPUnit invocation. Most of the tests are Skipped.

Tests PHP 5.3-7.4, PHP 8.0 is removed.

Extracted from #34

- "7.1"
- "7.2"
- "7.3"
- "7.4"
- "8.0"
Copy link
Contributor Author

@glensc glensc Jan 12, 2021

Choose a reason for hiding this comment

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

Something funny happens, PHPUnit shows progress until 89%:

the output contains several E-s, but the build is successful! this is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've ended up running same tests locally via docker, and there phpunit stops at 90%

Copy link
Contributor Author

@glensc glensc Jan 12, 2021

Choose a reason for hiding this comment

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

@falkenhawk what you think of disabling 8.0 for github actions, I ended up cherry-picking commits from the 8.0 branch, and seems it's still having issues. I don't want to make the PR itself very big, but rather make several PR's that are easier review and merge. possibly doing things in parallel, once there's basic phpunit invocation present.

or alternatively, can just ignore errors from 8.0 phpunit, merge with failing pipeline.

I propose making this PR exclude any issues with 8.0 (deal them later), either by removing 8.0 or consciously ignoring errors from 8.0 job.

ps: github actions does not have "allow_failure" equivalent yet. I've been tracking this issue for it: https://github.com/actions/toolkit/issues/399.

your input needed how to proceed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my next PR plans(after this PR) are (from #34):

  • add locale tests and locale fixes
  • add services, mysql, pgsql

Copy link
Member

Choose a reason for hiding this comment

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

wow I greatly appreciate your dedication.
Let's skip 8.0 for now and ditch any compatibility fixes needed for 8.0 here and deal with that in a separate PR, possibly branched off the 8.0 compatibility patch branch.

Having the whole pipeline running for php 5.3-7.4 on GHA would be awesome, as the first step. I asked travis for free OSS credits 2 weeks ago - because the jobs are currently not running at all due to depleted credits - and still no answer 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't know Travis OSS credits have been limited, I'm not following the news apparently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that enabling locales broken sql tests:

so, need to enable sql testing next, then locales testing

@glensc glensc force-pushed the gh-actions-minimal branch from 78eb4f3 to 7f77b8c Compare January 12, 2021 17:35
@glensc glensc force-pushed the gh-actions-minimal branch from 7f77b8c to 8247c9c Compare January 12, 2021 17:55
@glensc glensc marked this pull request as ready for review January 12, 2021 17:56
@glensc
Copy link
Contributor Author

glensc commented Jan 12, 2021

Updated to test only 5.3 to 7.4

Copy link
Member

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

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

Thank you!

btw are stty: 'standard input': Inappropriate ioctl for device printed out during tests because of --verbose phpunit flag, or is it something else, yet to be figured out?

@glensc
Copy link
Contributor Author

glensc commented Jan 12, 2021

haven't checked those stty messages. Since these do not trigger errors, I assumed they've always been there.

@falkenhawk
Copy link
Member

I haven't seen them before, but it's okay to check them later.

@falkenhawk falkenhawk merged commit 32e7a6a into zf1s:master Jan 12, 2021
@glensc glensc deleted the gh-actions-minimal branch January 12, 2021 18:26
glensc added a commit to glensc/php-zf1s that referenced this pull request Jan 12, 2021
* upstream/master:
  Add minimal phpunit run in GitHub Actions (zf1s#44)

Conflicts:
	.github/workflows/tests.yml
@glensc
Copy link
Contributor Author

glensc commented Jan 12, 2021

@falkenhawk please could you not do squash merges, please!

@falkenhawk
Copy link
Member

Okay I will keep that in mind, but could you tell me please why you don't like it? In this case I thought it's fine to have only one nice commit

@glensc
Copy link
Contributor Author

glensc commented Jan 26, 2021

@falkenhawk, I follow:

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.

2 participants