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

added experimental php 8.0 and elasticsearch 8.0 #2123

Merged
merged 17 commits into from Apr 4, 2023

Conversation

oleg-andreyev
Copy link
Contributor

@oleg-andreyev oleg-andreyev commented Nov 13, 2022

in order to move forward this #2011 created CI step, "experimental" (allow to fail) to collect information.

@oleg-andreyev oleg-andreyev force-pushed the php-8.0-es-8.0-expertimental branch 2 times, most recently from ab6923e to d0a1574 Compare November 13, 2022 21:52
@ruflin
Copy link
Owner

ruflin commented Nov 14, 2022

@oleg-andreyev Thanks for getting this going. It will help a lot to get rid of the most obvious issues quickly.

@VincentLanglet
Copy link
Contributor

Hi @oleg-andreyev, do you have time to finish this PR ? What is missing ?

@oleg-andreyev
Copy link
Contributor Author

@VincentLanglet last time everything was failing, today I've reconfigured a bit CI and now tests are working, it's only matter of fixing them.

@ruflin
Copy link
Owner

ruflin commented Mar 21, 2023

@oleg-andreyev Thanks for pushing on this front. Looks like there is only 1 failure left?

There was 1 failure:

1) Elastica\Test\Aggregation\RangeTest::testRangeAggregation
Failed asserting that 3 matches expected 2.

/home/runner/work/Elastica/Elastica/tests/Aggregation/RangeTest.php:28

--

There were 6 skipped tests:

1) Elastica\Test\Cluster\SettingsTest::testSetTransient
discovery.zen.minimum_master_nodes is deprecated, ignored in 7.x and removed in 8.x, see: https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-8.0.html#breaking-changes-8.0

/home/runner/work/Elastica/Elastica/tests/Cluster/SettingsTest.php:21

2) Elastica\Test\Cluster\SettingsTest::testSetPersistent
discovery.zen.minimum_master_nodes is deprecated, ignored in 7.x and removed in 8.x, see: https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-8.0.html#breaking-changes-8.0

/home/runner/work/Elastica/Elastica/tests/Cluster/SettingsTest.php:47

3) Elastica\Test\IndexTest::testForcemerge
Failed asserting that 2 is identical to 0.

/home/runner/work/Elastica/Elastica/tests/IndexTest.php:863

4) Elastica\Test\Multi\SearchTest::testSearchWithError
Elastica\Exception\ResponseException: [size] parameter cannot be negative, found [-2]

/home/runner/work/Elastica/Elastica/tests/Multi/SearchTest.php:295

5) Elastica\Test\Multi\SearchTest::testSearchWithErrorWithKeys
Elastica\Exception\ResponseException: [size] parameter cannot be negative, found [-2]

/home/runner/work/Elastica/Elastica/tests/Multi/SearchTest.php:[34](https://github.com/ruflin/Elastica/actions/runs/4462337031/jobs/7836801245?pr=2123#step:7:35)0

6) Elastica\Test\SnapshotTest::testSnapshotAndRestore
Failed asserting that actual size 2 matches expected size 1.

/home/runner/work/Elastica/Elastica/tests/SnapshotTest.php:76

FAILURES!
Tests: [48](https://github.com/ruflin/Elastica/actions/runs/4462337031/jobs/7836801245?pr=2123#step:7:49)6, Assertions: 2094, Failures: 1, Skipped: 6.

Generating code coverage report in Clover XML format ... done [00:00.1[68](https://github.com/ruflin/Elastica/actions/runs/4462337031/jobs/7836801245?pr=2123#step:7:69)]
Error: Process completed with exit code 1.

@oleg-andreyev
Copy link
Contributor Author

@ruflin @VincentLanglet all green. Expect random failure.

@ruflin
Copy link
Owner

ruflin commented Mar 23, 2023

🥳 What are the random failures you are referring to? Codecov or are there some flaky tests?

@oleg-andreyev
Copy link
Contributor Author

oleg-andreyev commented Mar 24, 2023

🥳 What are the random failures you are referring to? Codecov or are there some flaky tests?

It was codecov. It failed to validate signature , I'd propose to mark this step to allow to fail in order failing entire CI

environment: &environment
node.name: es01
cluster.name: es-docker-cluster
cluster.initial_master_nodes: es01
discovery.seed_hosts: es01
bootstrap.memory_lock: 'true'
xpack.security.enabled: 'false'
action.destructive_requires_name: 'false'
indices.id_field_data.enabled: 'true'
Copy link
Owner

Choose a reason for hiding this comment

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

If we didn't enable this, did some of the test fail?

@@ -88,11 +88,13 @@ protected function _getIndexForTest(): Index
$index = $this->_createIndex();

$index->addDocuments([
// this is not a mistake, dynamic mapping will take first type
Copy link
Owner

Choose a reason for hiding this comment

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

Make sense, that would also explain why on line 28 it was 2 before, so 1 doc got dropped I guess. Ideally we would load the mapping ourselfs instead of relying on dynamic mappings. Glad you found this.

@ruflin
Copy link
Owner

ruflin commented Apr 4, 2023

Getting this in to get us closer to the 8.0 release. I don't think in this PR is anything controversial around 8.0. @oleg-andreyev Appreciate that you took this on and on the way also fixed some of the test and did cleanup 👍

@ruflin ruflin merged commit 96f8c9d into ruflin:8.x Apr 4, 2023
8 of 9 checks passed
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.

None yet

3 participants