Problem/Motivation

Postponed until #3270464: Investigate search_api_opensearch as base for elasticsearch_connector is merged.

tests/src/Kernel/ElasticSearchBackendTest.php has few @todo, since the tests differ from search_api_opensearch (https://git.drupalcode.org/project/search_api_opensearch/-/blob/2.x/test...), figure out why and adjust the commented tests to pass.

See the comments on https://git.drupalcode.org/project/elasticsearch_connector/-/merge_reque...

Proposed resolution

Fix the tests.

Remaining tasks

  1. Write a patch
  2. Review and feedback
  3. RTBC and feedback
  4. Commit
  5. Release — released in 8.0.0-alpha1

User interface changes

None.

API changes

  1. Changes the signature of \Drupal\elasticsearch_connector\SearchAPI\Query\FacetParamBuilder's ::buildFacetParams(), and ::buildTermBucketAgg().
  2. Changes the signature of \Drupal\elasticsearch_connector\SearchAPI\Query\FilterBuilder::buildFilterTerm()

Data model changes

None.

Issue fork elasticsearch_connector-3426827

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sokru created an issue. See original summary.

sokru’s picture

sokru commented
Issue summary: View changes
sokru’s picture

sokru commented
Issue summary: View changes
Status: Postponed » Active

hexaki made their first commit to this issue’s fork.

hexaki opened merge request !42

hexaki’s picture

hexaki commented

Hi, nice work for the recent updates of this module!

I think that elasticsearch_connector should be able to validate all tests from BackendTestBase.

My changes:
- call the parent testBackend
- fix the code so that all of these tests pass

Todo:
- Fix the unit tests with the changes I made
- Add PHPUnit\Asynchronicity back

Some notes on my changes:
- Some issues on the results were due to the fuzziness, I think a new issue for testing this features could be interesting
- The tests from searchSuccess where duplicate form the parent, I remove them

I would love your feedback.

hexaki’s picture

hexaki commented
Status: Active » Needs review

I've removed the need for PHPUnit\Asynchronicity.
And updated the unit tests.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Parent issue: » #3427243: Plan for 8.0.0-alpha release

Adding #3427243: Plan for 8.0.0-alpha release as a parent issue.

sokru’s picture

sokru commented
Status: Needs review » Reviewed & tested by the community

Thanks @hexaki! I have looked the MR few times, on the first glance it looked like the changes where totally out-of-scope, but after more reading they make totally sense, excellent work! Its good that we're be able to also cover the parent testBackend! We should include that as release highlights for 8.0.0.

Only minor nitpick about the quotes,

Drupal does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.
With that caveat in mind, single quote strings should be used by default.

https://www.drupal.org/docs/develop/standards/php/php-coding-standards#:....

I've set the status RTBC, does not hurt if anyone else could also check the changes, I'll re-read the changes once more before committing.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

This looks good to me as well. Excellent work, @hexaki: thank you very much!

Note that when we merge this, we should change the status of #3426826: Investigate need of matthiasnoback/phpunit-asynchronicity to replace sleep() in tests to "Closed (outdated)"

  • sokru committed cecba8c2 on 8.0.x authored by hexaki 
    Issue #3426827 by hexaki, sokru, mparker17: Figure out why test results...
sokru’s picture

sokru commented
Status: Reviewed & tested by the community » Fixed

Changed few double quotation marks, but changing all of them would make Elasticsearch tests to fail.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Update the issue summary to document changes made in this ticket.