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.

Sign up for GitHub

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

Jump to bottom

added experimental php 8.0 and elasticsearch 8.0 #2123

Merged
merged 17 commits into from
Apr 4, 2023
Merged

added experimental php 8.0 and elasticsearch 8.0 #2123

merged 17 commits into from
Apr 4, 2023

Conversation

oleg-andreyev
Copy link
Contributor

@oleg-andreyev oleg-andreyev commented Nov 13, 2022 •
edited
Loading

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 oleg-andreyev force-pushed the php-8.0-es-8.0-expertimental branch from 2e96cee to d48b61f Compare March 19, 2023 10:10
@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 •
edited
Loading

🥳 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

ruflin
ruflin reviewed Apr 4, 2023
View reviewed changes
docker/docker-compose.es.yml
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?

ruflin
ruflin reviewed Apr 4, 2023
View reviewed changes
tests/Aggregation/RangeTest.php
@@ -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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin left review comments

@VincentLanglet VincentLanglet VincentLanglet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oleg-andreyev @ruflin @VincentLanglet