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

Replace Travis with GitHub Actions #34

Closed
wants to merge 57 commits into from
Closed

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Dec 3, 2020

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

@falkenhawk can you check if GitHub Actions need to be enabled for this to work?

@glensc glensc marked this pull request as ready for review December 3, 2020 08:26
@falkenhawk
Copy link
Member

falkenhawk commented Dec 3, 2020

hi Elan, I don't see any specific option to enable for gha to run here. They are enabled by default. Perhaps there is a setting to toggle on your fork?
Or try with pull_request_target maybe? https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/#improvements-for-public-repository-forks

Do you think there might be a chance to have GHA running for full range 5.3-8.0 of php versions, as it was on travis?

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

1) Zend_Ldap_ConverterTest::testToLdapSerialize with data set #2 ('O:8:"DateTime":3:{s:4:"date";s:19:"1970-01-01 00:00:00";s:13:"timezone_type";i:1;s:8:"timezone";s:6:"+00:00";}', DateTime)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'O:8:"DateTime":3:{s:4:"date";s:19:"1970-01-01 00:00:00";s:13:"timezone_type";i:1;s:8:"timezone";s:6:"+00:00";}'
+'O:8:"DateTime":3:{s:4:"date";s:26:"1970-01-01 00:00:00.000000";s:13:"timezone_type";i:1;s:8:"timezone";s:6:"+00:00";}'

the other versions pass (note this is without mysql and pgsql yet):

I wonder, is it related to some php.ini setting?

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

@falkenhawk do you have a list of PHP extensions needed for the test suite? from Travis config I see memcache, memcached, and ldap are added, but no info about other already present extensions.

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

Seems Zend_Ldap_ConverterTest::testToLdapSerialize was already broken, just earlier skipped with TRAVIS env, need to change those to CI env check.

eventually, remove the Travis specific exceptions, perhaps the input data could be all set same for all versions. but I'm leaving out that for now out of this PR.

@glensc glensc marked this pull request as draft December 3, 2020 10:26
@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

any ideas on this one?

1) Zend_Db_Table_Select_MysqliTest::testSelectWhereWithTypeFloat
Zend_Db_Select::_join(): Error occurred while closing statement

/home/runner/work/php-zf1s/php-zf1s/packages/zend-db/library/Zend/Db/Select.php:852
/home/runner/work/php-zf1s/php-zf1s/packages/zend-db/library/Zend/Db/Select.php:251
/home/runner/work/php-zf1s/php-zf1s/tests/Zend/Db/Select/TestCommon.php:859
/home/runner/work/php-zf1s/php-zf1s/tests/Zend/Db/Select/TestCommon.php:876

happens with 5.5 and 5.4 only:

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

  1. Zend_Db_Table_Select_MysqliTest::testSelectWhereWithTypeFloat
    Zend_Db_Select::_join(): Error occurred while closing statement

this makes no sense:

how can array value assignment produce such error? all the values are variables, no function calls:

            $this->_parts[self::FROM][$correlationName] = array(
                'joinType'      => $type,
                'schema'        => $schema,
                'tableName'     => $tableName,
                'joinCondition' => $cond
                );

I can not find any match for such error message in zf1s codebase...

@glensc
Copy link
Contributor Author

glensc commented Dec 3, 2020

Seems the error comes from ext/mysqli:

@falkenhawk what can we do here with that?

This is to clearly indicate specific encoding is expected
@glensc
Copy link
Contributor Author

glensc commented Dec 7, 2020

1) Zend_Locale_FormatTest::testToFloatSetlocale
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1 234,50'
+'123 4,5,00'

/home/runner/work/php-zf1s/php-zf1s/tests/Zend/Locale/FormatTest.php:947

locale test goes totally bizarre after installing required locales. note two commas in value!

the input number is $myFloat = 1234.5;

it fails in all PHP versions except 7.4!

@glensc
Copy link
Contributor Author

glensc commented Dec 7, 2020

seems enabling locale tests by installing needed locales, brings more trouble than needed.

the tests are not run in proper isolation, i.e global resource like locale must be restored after a test, and that is not performed, neither are tests run in isolation (i think phpunit 3.7 does not even support that), and sql queries are written with very bad style, resulting syntax error as numeric value value formatted with comma according to locale rules, gets parsed as two values in sql.

2) Zend_Db_Table_Select_MysqliTest::testSelectWhereOr
Zend_Db_Exception: SQL error for "INSERT INTO `zfprice` (`product_id`, `price_name`, `price_total`) VALUES (1, 'Price 1', 200,45)": Column count doesn't match value count at row 1

@falkenhawk
Copy link
Member

falkenhawk commented Dec 8, 2020

Whoa that's some struggle here. You are doing a tremendous job @glensc thank you!

I am sorry I cannot be of much help here as I am currently busy with other work-related stuff...
Perhaps keep Travis enabled in parallel until you're satisfied with the GHA result? yes let's still have travis running (as long as it works, but who knows for how long - I can sense some bad vibes since they were aquired by another company - it looks like they are on a downward spiral only to be shut down at some point soon - note that's only my guess) - until the point we are satisfied with the GHA result, but please continue on this PR as long as you have resources for it and feel that motivation 🙈

How come Travis had not been throwing such errors before 🤔 (locale, db, etc.)

@glensc
Copy link
Contributor Author

glensc commented Dec 8, 2020

My guess why Travis did not fail on these, as those tests were skipped.

The ext/mysqli failure, is my guess that 5.4 and 5.5 have some bug, which did not manifest on Travis built PHP (different OS, different compiler) but could be that maybe ext/mysqli tests were not ran in Travis.

I've also felt Travis downfall, in projects I was forced to switch to GHA because Travis's pipeline was in the queue for days, which is not acceptable. And at some point the Travis integration was not visible in GitHub at all, had to contact Travis support, and they said I need to remove the token and setup Integration again.

* master:
  Setup GitHub Actions to lint all files in the repository (zf1s#37)
  Add clarifying parens for nested ternary operation (zf1s#39)
  Convert all *.php files to Unix line encoding (zf1s#38)
  Remove invalid unset $this operation (zf1s#36)
* master:
  Move composer.json validate of sub-packages to GitHub Actions
  Disable ext-xdebug to remove composer xdebug warning
* master:
  Use versioned dependencies in composer.json
  Install composer dependencies in CI
  Also validate composer.json of the main package
  Add required description/license to composer.json
  Backport test for CVE-2021-3007
  Backport of fix for CVE-2021-3007 in Zend_Http_Response_Stream
* upstream/master:
  Add minimal phpunit run in GitHub Actions (zf1s#44)

Conflicts:
	.github/workflows/tests.yml
* upstream/master:
  Disable memcached tests in Travis
  Setup TestConfiguration for CI to enable memcache tests
  Add Memcache service to GitHub Actions
  Rename tests/TestConfiguration.php.dist to tests/TestConfiguration.dist.php
@falkenhawk
Copy link
Member

Current status - travis: (php 8.0)

OK, but incomplete or skipped tests!

Tests: 15522, Assertions: 61406, Incomplete: 22, Skipped: 607.

GHA: (php 8.0)

OK, but incomplete or skipped tests!
Tests: 15572, Assertions: 61706, Incomplete: 21, Skipped: 586.

There are even less tests skipped on GHA compared to travis. 🎉
Thank you @glensc for your excellent work here. It would not be possible without your contributions!

So I think the migration can be considered as complete now and travis config can be deleted?

@glensc
Copy link
Contributor Author

glensc commented Sep 27, 2021

@falkenhawk thanks for your kind words. yes, Travis can be deleted.

not sure about this PR in particular, here may be some changes that haven't been extracted to separate PR. This PR itself is not mergeable, and rebase is not worthy of it. but could do another merge commit here and see what changes remained.

@glensc
Copy link
Contributor Author

glensc commented Sep 27, 2021

From the top of my head I recall problematic were MySQL, locale tests. Actually locale tests are GHA only, they were not properly done in Travis.

I think memcache and pgsql may be still missing from GitHub Actions.

Conflicts:
	.github/workflows/tests.yml
	.travis.yml
	tests/Zend/Locale/FormatTest.php
	tests/Zend/Validate/FloatTest.php
@glensc
Copy link
Contributor Author

glensc commented Sep 27, 2021

Merged changes from origin/master. Some changes remaining, if you wish you could cherry-pick them. But seems mostly those are just because the changes extracted here were solved differently after review. And some spelling fixes and missing properties are not that important.

@glensc glensc closed this Sep 27, 2021
@glensc glensc reopened this Sep 27, 2021
@glensc glensc closed this Sep 27, 2021
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