From: Sarah Hoffmann Date: Wed, 31 Jul 2024 15:02:39 +0000 (+0200) Subject: Merge pull request #3493 from lonvia/clean-up-bdd-tests X-Git-Tag: deploy~4^2~3 X-Git-Url: https://git.openstreetmap.org/nominatim.git/commitdiff_plain/a4d7cdd2ad7b1e087751e55a56454781aa6580bc?hp=67462e095319038fa53636decb69dcf40f0c6dee Merge pull request #3493 from lonvia/clean-up-bdd-tests Various cleanups of BDD tests --- diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml index 5b7dacf0..1cfaf616 100644 --- a/.github/workflows/ci-tests.yml +++ b/.github/workflows/ci-tests.yml @@ -118,7 +118,8 @@ jobs: - name: BDD tests run: | - python3 -m behave -DREMOVE_TEMPLATE=1 -DBUILDDIR=$GITHUB_WORKSPACE/build --format=progress3 + export PATH=$GITHUB_WORKSPACE/build/osm2pgsql:$PATH + python3 -m behave -DREMOVE_TEMPLATE=1 --format=progress3 working-directory: Nominatim/test/bdd - name: Install mypy and typechecking info @@ -170,7 +171,8 @@ jobs: - name: BDD tests (legacy tokenizer) run: | - python3 -m behave -DREMOVE_TEMPLATE=1 -DBUILDDIR=$GITHUB_WORKSPACE/build -DAPI_ENGINE=php -DTOKENIZER=legacy --format=progress3 + export PATH=$GITHUB_WORKSPACE/build/osm2pgsql:$PATH + python3 -m behave -DREMOVE_TEMPLATE=1 -DSERVER_MODULE_PATH=$GITHUB_WORKSPACE/build/module -DAPI_ENGINE=php -DTOKENIZER=legacy --format=progress3 working-directory: Nominatim/test/bdd @@ -217,7 +219,8 @@ jobs: - name: BDD tests (php) run: | - python3 -m behave -DREMOVE_TEMPLATE=1 -DBUILDDIR=$GITHUB_WORKSPACE/build -DAPI_ENGINE=php --format=progress3 + export PATH=$GITHUB_WORKSPACE/build/osm2pgsql:$PATH + python3 -m behave -DREMOVE_TEMPLATE=1 -DAPI_ENGINE=php --format=progress3 working-directory: Nominatim/test/bdd diff --git a/docs/develop/Development-Environment.md b/docs/develop/Development-Environment.md index a7c474e8..19948e7c 100644 --- a/docs/develop/Development-Environment.md +++ b/docs/develop/Development-Environment.md @@ -50,6 +50,10 @@ The documentation is built with mkdocs: * [mkdocstrings](https://mkdocstrings.github.io/) >= 0.25 * [mkdocs-material](https://squidfunk.github.io/mkdocs-material/) +Please be aware that tests always run against the globally installed +osm2pgsql, so you need to have this set up. If you want to test against +the vendored version of osm2pgsql, you need to set the PATH accordingly. + ### Installing prerequisites on Ubuntu/Debian The Python tools should always be run with the most recent version. diff --git a/docs/develop/Testing.md b/docs/develop/Testing.md index 97d40ab7..c220f4e4 100644 --- a/docs/develop/Testing.md +++ b/docs/develop/Testing.md @@ -78,7 +78,6 @@ To run the functional tests, do The tests can be configured with a set of environment variables (`behave -D key=val`): - * `BUILDDIR` - build directory of Nominatim installation to test * `TEMPLATE_DB` - name of template database used as a skeleton for the test databases (db tests) * `TEST_DB` - name of test database (db tests) @@ -91,7 +90,7 @@ The tests can be configured with a set of environment variables (`behave -D key= * `DB_USER` - (optional) username of database login * `DB_PASS` - (optional) password for database login * `SERVER_MODULE_PATH` - (optional) path on the Postgres server to Nominatim - module shared library file + module shared library file (only needed for legacy tokenizer) * `REMOVE_TEMPLATE` - if true, the template and API database will not be reused during the next run. Reusing the base templates speeds up tests considerably but might lead to outdated errors @@ -122,23 +121,6 @@ and compromises the following data: API tests should only be testing the functionality of the website PHP code. Most tests should be formulated as BDD DB creation tests (see below) instead. -#### Code Coverage (PHP engine only) - -The API tests also support code coverage tests. You need to install -[PHP_CodeCoverage](https://github.com/sebastianbergmann/php-code-coverage). -On Debian/Ubuntu run: - - apt-get install php-codecoverage php-xdebug - -Then run the API tests as follows: - - behave api -DPHPCOV= - -The output directory must be an absolute path. To generate reports, you can use -the [phpcov](https://github.com/sebastianbergmann/phpcov) tool: - - phpcov merge --html= - ### DB Creation Tests (`test/bdd/db`) These tests check the import and update of the Nominatim database. They do not diff --git a/src/nominatim_api/search/db_search_builder.py b/src/nominatim_api/search/db_search_builder.py index e29f0b93..6453509e 100644 --- a/src/nominatim_api/search/db_search_builder.py +++ b/src/nominatim_api/search/db_search_builder.py @@ -167,7 +167,12 @@ class SearchBuilder: expected_count = sum(t.count for t in hnrs) partials = {t.token: t.addr_count for trange in address - for t in self.query.get_partials_list(trange)} + for t in self.query.get_partials_list(trange) + if t.is_indexed} + + if not partials: + # can happen when none of the partials is indexed + return if expected_count < 8000: sdata.lookups.append(dbf.FieldLookup('nameaddress_vector', diff --git a/src/nominatim_api/search/legacy_tokenizer.py b/src/nominatim_api/search/legacy_tokenizer.py index ecb0bbfe..c7b10119 100644 --- a/src/nominatim_api/search/legacy_tokenizer.py +++ b/src/nominatim_api/search/legacy_tokenizer.py @@ -193,7 +193,7 @@ class LegacyQueryAnalyzer(AbstractQueryAnalyzer): lookup_word = row.word_token[1:] elif rowclass == 'place' and row.type == 'postcode': ttype = qmod.TokenType.POSTCODE - lookup_word = row.word_token[1:] + lookup_word = row.word else: ttype = qmod.TokenType.NEAR_ITEM if row.operator in ('in', 'near')\ else qmod.TokenType.QUALIFIER @@ -253,13 +253,14 @@ class LegacyQueryAnalyzer(AbstractQueryAnalyzer): def _dump_word_tokens(query: qmod.QueryStruct) -> Iterator[List[Any]]: - yield ['type', 'token', 'word_token', 'lookup_word', 'penalty', 'count', 'info'] + yield ['type', 'token', 'word_token', 'lookup_word', 'penalty', 'count', 'info', 'indexed'] for node in query.nodes: for tlist in node.starting: for token in tlist.tokens: t = cast(LegacyToken, token) yield [tlist.ttype.name, t.token, t.word_token or '', - t.lookup_word or '', t.penalty, t.count, t.info] + t.lookup_word or '', t.penalty, t.count, t.info, + 'Y' if t.is_indexed else 'N'] async def create_query_analyzer(conn: SearchConnection) -> AbstractQueryAnalyzer: diff --git a/test/bdd/api/search/postcode.feature b/test/bdd/api/search/postcode.feature index e372f449..827af1ea 100644 --- a/test/bdd/api/search/postcode.feature +++ b/test/bdd/api/search/postcode.feature @@ -3,11 +3,15 @@ Feature: Searches with postcodes Various searches involving postcodes + @v1-api-php-only Scenario: US 5+4 ZIP codes are shortened to 5 ZIP codes if not found When sending json search query "36067 1111, us" with address Then result addresses contain | postcode | | 36067 | + And results contain + | type | + | postcode | Scenario: Postcode search with address When sending json search query "9486, mauren" diff --git a/test/bdd/db/query/postcodes.feature b/test/bdd/db/query/postcodes.feature index 9f024959..fa4f6a0b 100644 --- a/test/bdd/db/query/postcodes.feature +++ b/test/bdd/db/query/postcodes.feature @@ -76,6 +76,7 @@ Feature: Querying fo postcode variants | AD675 | + @fail-legacy Scenario: Different postcodes with the same normalization can both be found Given the places | osm | class | type | addr+postcode | addr+housenumber | geometry | diff --git a/test/bdd/db/query/reverse.feature b/test/bdd/db/query/reverse.feature new file mode 100644 index 00000000..12941102 --- /dev/null +++ b/test/bdd/db/query/reverse.feature @@ -0,0 +1,23 @@ +@DB +Feature: Reverse searches + Test results of reverse queries + + @v1-api-python-only + Scenario: POI in POI area + Given the 0.0001 grid with origin 1,1 + | 1 | | | | | | | | 2 | + | | 9 | | | | | | | | + | 4 | | | | | | | | 3 | + And the places + | osm | class | type | geometry | + | W1 | aeroway | terminal | (1,2,3,4,1) | + | N1 | amenity | restaurant | 9 | + When importing + And sending v1/reverse at 1.0001,1.0001 + Then results contain + | osm | + | N1 | + When sending v1/reverse at 1.0003,1.0001 + Then results contain + | osm | + | W1 | diff --git a/test/bdd/db/query/search_simple.feature b/test/bdd/db/query/search_simple.feature index 5fef3132..270d2e55 100644 --- a/test/bdd/db/query/search_simple.feature +++ b/test/bdd/db/query/search_simple.feature @@ -77,6 +77,7 @@ Feature: Searching of simple objects | W1 | + @fail-legacy Scenario Outline: Special cased american states will be found Given the grid | 1 | | 2 | diff --git a/test/bdd/db/update/linked_places.feature b/test/bdd/db/update/linked_places.feature index 9204b3bb..d6370ebb 100644 --- a/test/bdd/db/update/linked_places.feature +++ b/test/bdd/db/update/linked_places.feature @@ -157,17 +157,17 @@ Feature: Updates of linked places | R1 | boundary | administrative | rel | 8 | (10,11,12,13,10) | And the places | osm | class | type | name+name:de | - | N3 | place | city | pnt | + | N3 | place | city | greeny | And the relations | id | members | | 1 | N3:label | When importing Then placex contains | object | linked_place_id | name+_place_name:de | - | R1 | - | pnt | + | R1 | - | greeny | And placex contains | object | linked_place_id | name+name:de | - | N3 | R1 | pnt | + | N3 | R1 | greeny | When updating places | osm | class | type | name+name:de | | N3 | place | city | newname | @@ -188,18 +188,18 @@ Feature: Updates of linked places | R1 | boundary | administrative | rel | 8 | (10,11,12,13,10) | And the places | osm | class | type | name | - | N3 | place | city | pnt | + | N3 | place | city | greeny | And the relations | id | members | | 1 | N3:label | When importing Then placex contains | object | linked_place_id | name+_place_name | name+name | - | R1 | - | pnt | rel | + | R1 | - | greeny | rel | And placex contains | object | linked_place_id | name+name | - | N3 | R1 | pnt | - When sending search query "pnt" + | N3 | R1 | greeny | + When sending search query "greeny" Then results contain | osm | | R1 | @@ -212,7 +212,7 @@ Feature: Updates of linked places And placex contains | object | linked_place_id | name+_place_name:de | name+name | | R1 | - | depnt | rel | - When sending search query "pnt" + When sending search query "greeny" Then exactly 0 results are returned Scenario: Updating linkee extratags keeps linker's extratags diff --git a/test/bdd/environment.py b/test/bdd/environment.py index 85896898..155b8d90 100644 --- a/test/bdd/environment.py +++ b/test/bdd/environment.py @@ -5,16 +5,18 @@ # Copyright (C) 2024 by the Nominatim developer community. # For a full list of authors see the git log. from pathlib import Path +import sys from behave import * +sys.path.insert(1, str(Path(__file__, '..', '..', '..', 'src').resolve())) + from steps.geometry_factory import GeometryFactory from steps.nominatim_environment import NominatimEnvironment -TEST_BASE_DIR = Path(__file__) / '..' / '..' +TEST_BASE_DIR = Path(__file__, '..', '..').resolve() userconfig = { - 'BUILDDIR' : (TEST_BASE_DIR / '..' / 'build').resolve(), 'REMOVE_TEMPLATE' : False, 'KEEP_TEST_DB' : False, 'DB_HOST' : None, @@ -24,12 +26,11 @@ userconfig = { 'TEMPLATE_DB' : 'test_template_nominatim', 'TEST_DB' : 'test_nominatim', 'API_TEST_DB' : 'test_api_nominatim', - 'API_TEST_FILE' : (TEST_BASE_DIR / 'testdb' / 'apidb-test-data.pbf').resolve(), + 'API_TEST_FILE' : TEST_BASE_DIR / 'testdb' / 'apidb-test-data.pbf', 'SERVER_MODULE_PATH' : None, 'TOKENIZER' : None, # Test with a custom tokenizer 'STYLE' : 'extratags', - 'API_ENGINE': 'falcon', - 'PHPCOV' : False, # set to output directory to enable code coverage + 'API_ENGINE': 'falcon' } use_step_matcher("re") diff --git a/test/bdd/steps/cgi-with-coverage.php b/test/bdd/steps/cgi-with-coverage.php deleted file mode 100644 index dbd8993b..00000000 --- a/test/bdd/steps/cgi-with-coverage.php +++ /dev/null @@ -1,40 +0,0 @@ -stop(); - $writer = new \SebastianBergmann\CodeCoverage\Report\PHP; - $writer->process($oCoverage, $_SERVER['PHP_CODE_COVERAGE_FILE']); -} - -$covfilter = new SebastianBergmann\CodeCoverage\Filter(); -if (method_exists($covfilter, 'addDirectoryToWhitelist')) { - // pre PHPUnit 9 - $covfilter->addDirectoryToWhitelist($_SERVER['COV_PHP_DIR'].'/lib-php'); - $covfilter->addDirectoryToWhitelist($_SERVER['COV_PHP_DIR'].'/website'); - $coverage = new SebastianBergmann\CodeCoverage\CodeCoverage(null, $covfilter); -} else { - // since PHP Uit 9 - $covfilter->includeDirectory($_SERVER['COV_PHP_DIR'].'/lib-php'); - $covfilter->includeDirectory($_SERVER['COV_PHP_DIR'].'/website'); - $coverage = new SebastianBergmann\CodeCoverage\CodeCoverage( - (new SebastianBergmann\CodeCoverage\Driver\Selector)->forLineCoverage($covfilter), - $covfilter - ); -} - -$coverage->start($_SERVER['COV_TEST_NAME']); - -register_shutdown_function('coverage_shutdown', $coverage); - -include $_SERVER['COV_SCRIPT_FILENAME']; diff --git a/test/bdd/steps/nominatim_environment.py b/test/bdd/steps/nominatim_environment.py index 17a76745..c4b05588 100644 --- a/test/bdd/steps/nominatim_environment.py +++ b/test/bdd/steps/nominatim_environment.py @@ -6,14 +6,11 @@ # For a full list of authors see the git log. from pathlib import Path import importlib -import sys import tempfile import psycopg from psycopg import sql as pysql -sys.path.insert(1, str((Path(__file__) / '..' / '..' / '..' / '..'/ 'src').resolve())) - from nominatim_db import cli from nominatim_db.config import Configuration from nominatim_db.db.connection import Connection, register_hstore, execute_scalar @@ -26,7 +23,6 @@ class NominatimEnvironment: """ def __init__(self, config): - self.build_dir = Path(config['BUILDDIR']).resolve() self.src_dir = (Path(__file__) / '..' / '..' / '..' / '..').resolve() self.db_host = config['DB_HOST'] self.db_port = config['DB_PORT'] @@ -41,8 +37,6 @@ class NominatimEnvironment: self.server_module_path = config['SERVER_MODULE_PATH'] self.reuse_template = not config['REMOVE_TEMPLATE'] self.keep_scenario_db = config['KEEP_TEST_DB'] - self.code_coverage_path = config['PHPCOV'] - self.code_coverage_id = 1 self.default_config = Configuration(None).get_os_env() self.test_env = None @@ -56,6 +50,9 @@ class NominatimEnvironment: raise RuntimeError(f"Unknown API engine '{config['API_ENGINE']}'") self.api_engine = getattr(self, f"create_api_request_func_{config['API_ENGINE']}")() + if self.tokenizer == 'legacy' and self.server_module_path is None: + raise RuntimeError("You must set -DSERVER_MODULE_PATH when testing the legacy tokenizer.") + def connect_database(self, dbname): """ Return a connection to the database with the given name. Uses configured host, user and port. @@ -71,13 +68,6 @@ class NominatimEnvironment: dbargs['password'] = self.db_pass return psycopg.connect(**dbargs) - def next_code_coverage_file(self): - """ Generate the next name for a coverage file. - """ - fn = Path(self.code_coverage_path) / "{:06d}.cov".format(self.code_coverage_id) - self.code_coverage_id += 1 - - return fn.resolve() def write_nominatim_config(self, dbname): """ Set up a custom test configuration that connects to the given @@ -107,8 +97,6 @@ class NominatimEnvironment: self.test_env['NOMINATIM_DATADIR'] = str((self.src_dir / 'data').resolve()) self.test_env['NOMINATIM_SQLDIR'] = str((self.src_dir / 'lib-sql').resolve()) self.test_env['NOMINATIM_CONFIGDIR'] = str((self.src_dir / 'settings').resolve()) - self.test_env['NOMINATIM_DATABASE_MODULE_SRC_PATH'] = str((self.build_dir / 'module').resolve()) - self.test_env['NOMINATIM_OSM2PGSQL_BINARY'] = str((self.build_dir / 'osm2pgsql' / 'osm2pgsql').resolve()) if self.tokenizer is not None: self.test_env['NOMINATIM_TOKENIZER'] = self.tokenizer if self.import_style is not None: @@ -116,9 +104,6 @@ class NominatimEnvironment: if self.server_module_path: self.test_env['NOMINATIM_DATABASE_MODULE_PATH'] = self.server_module_path - else: - # avoid module being copied into the temporary environment - self.test_env['NOMINATIM_DATABASE_MODULE_PATH'] = str((self.build_dir / 'module').resolve()) if self.website_dir is not None: self.website_dir.cleanup() @@ -137,8 +122,7 @@ class NominatimEnvironment: def get_test_config(self): cfg = Configuration(Path(self.website_dir.name), environ=self.test_env) - cfg.set_libdirs(module=self.build_dir / 'module', - osm2pgsql=self.build_dir / 'osm2pgsql' / 'osm2pgsql') + cfg.set_libdirs(module=self.server_module_path) return cfg def get_libpq_dsn(self): @@ -305,8 +289,8 @@ class NominatimEnvironment: if self.website_dir is not None: cmdline = list(cmdline) + ['--project-dir', self.website_dir.name] - cli.nominatim(module_dir='', - osm2pgsql_path=str(self.build_dir / 'osm2pgsql' / 'osm2pgsql'), + cli.nominatim(module_dir=self.server_module_path, + osm2pgsql_path=None, cli_args=cmdline, environ=self.test_env) diff --git a/test/bdd/steps/steps_api_queries.py b/test/bdd/steps/steps_api_queries.py index aa1b43b8..93501e42 100644 --- a/test/bdd/steps/steps_api_queries.py +++ b/test/bdd/steps/steps_api_queries.py @@ -114,18 +114,7 @@ def send_api_query_php(endpoint, params, context): for k, v in context.http_headers.items(): env['HTTP_' + k.upper().replace('-', '_')] = v - cmd = ['/usr/bin/env', 'php-cgi', '-f'] - if context.nominatim.code_coverage_path: - env['XDEBUG_MODE'] = 'coverage' - env['COV_SCRIPT_FILENAME'] = env['SCRIPT_FILENAME'] - env['COV_PHP_DIR'] = context.nominatim.src_dir - env['COV_TEST_NAME'] = f"{context.scenario.filename}:{context.scenario.line}" - env['SCRIPT_FILENAME'] = \ - os.path.join(os.path.split(__file__)[0], 'cgi-with-coverage.php') - cmd.append(env['SCRIPT_FILENAME']) - env['PHP_CODE_COVERAGE_FILE'] = context.nominatim.next_code_coverage_file() - else: - cmd.append(env['SCRIPT_FILENAME']) + cmd = ['/usr/bin/env', 'php-cgi', '-f', env['SCRIPT_FILENAME']] for k,v in params.items(): cmd.append(f"{k}={v}") diff --git a/test/bdd/steps/steps_osm_data.py b/test/bdd/steps/steps_osm_data.py index 0c1b421d..4cee75f7 100644 --- a/test/bdd/steps/steps_osm_data.py +++ b/test/bdd/steps/steps_osm_data.py @@ -16,7 +16,7 @@ from geometry_alias import ALIASES def get_osm2pgsql_options(nominatim_env, fname, append): return dict(import_file=fname, - osm2pgsql=str(nominatim_env.build_dir / 'osm2pgsql' / 'osm2pgsql'), + osm2pgsql='osm2pgsql', osm2pgsql_cache=50, osm2pgsql_style=str(nominatim_env.get_test_config().get_import_style_file()), osm2pgsql_style_path=nominatim_env.get_test_config().config_dir,