Repository navigation
contributing: Add a testing guide section on databases - #7961
Valyrian-Code wants to merge 7 commits into
Conversation
Covers what a test has to set up for attribute tables and for the temporal database: that the temporal database lives in the mapset and so is shared by a module-scoped session, that t.create creates it, and that grass.temporal reads the session from os.environ so a fixture has to mirror it with monkeypatch. Addresses the database half of OSGeo#7861. The network half is left for someone who has written such a test.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Revise the documented vector-table setup and temporal database prerequisite.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds database-focused testing guidance for GRASS contributors.
Changes:
- Documents vector attribute-table setup.
- Explains temporal database initialization and mapset scoping.
- Covers environment mirroring with
monkeypatch.
| File | Description |
|---|---|
doc/development/testing.md |
Adds the database testing guide section. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Clarified the relationship between vector geometry and attribute tables in tests, specifying the need to create the attribute table separately. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
||
| ### Tests which use a database | ||
|
|
||
| Vector geometry and attribute tables are separate. If a test needs attributes, create the map and its table (for example with _v.db.addtable_), then read the values back with _v.db.select_. Nothing else is needed once the table is linked. |
There was a problem hiding this comment.
Can you wrap it to stop our linters complaining?
|
The linked issue likely means databases as in Postgres, not merely testing the APIs which hide things like vector attribute storage. I did not eval how useful the current content is. |
tgis.init() creates the mapset's temporal database when there is none, and the t.* tools call it first, so the section was directing people to setup they do not need. Keep the location, since it explains why a module-scoped session shares one database. Also wrap the attribute paragraph.
|
You are right, the issue is about Postgres and this is not that. I had read it as the setup a fixture needs before touching stored attributes. Happy to narrow and retitle it, or close it and leave #7861 for someone who can write the Postgres part. Fixed the two Copilot points meanwhile: v.random really does give a map with no table, and tgis.init() creates the database itself, so t.create was never the prerequisite I claimed. Long line wrapped. |
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
I think it is useful, but just the sections need to be better and the current text confuses attributes and temporal databases. There are similarities - parallels even - but major differences, too.
The per-mapset isolation is common to both, although for attributes, the solution is either separate mapsets or actually not going with the defaults and switching to per-map database (I would be interested in seeing analysis for that, slower? breaks for some things?).
Otherwise, "test with the default backend unless testing the backend" is applicable to both vector attributes and temporal data (but I would think that's around where it ends, rest would be different, so again two sections are more appropriate).
| ### Tests which use a database | ||
|
|
||
| Vector geometry and attribute tables are separate. If a test needs | ||
| attributes, create the map and its table (for example with _v.db.addtable_), | ||
| then read the values back with _v.db.select_. Nothing else is needed once the | ||
| table is linked. |
There was a problem hiding this comment.
This piece is about testing vector with attributes (broader) or just testing of vector attributes (narrower).
There was a problem hiding this comment.
Narrowed to testing a vector with attributes. Says a map from v.random has no table, and that the first one defines the connection for the mapset.
| The temporal database does not need creating by hand: `tgis.init()` creates | ||
| one for the current mapset if there is none, and the `t.*` tools call it | ||
| before they do anything, so a fixture which creates a new mapset gets a | ||
| database on first use. By default it lives in that mapset, at | ||
| `$GISDBASE/$LOCATION_NAME/$MAPSET/tgis/sqlite.db`, which is worth knowing | ||
| because it means a module-scoped session shares one between its tests. For | ||
| tests about the connection itself, `temporal/t.connect/tests/conftest.py` | ||
| shows fixtures for the states worth covering: a mapset with a connection, one | ||
| without, and two mapsets which each have their own. |
There was a problem hiding this comment.
While this piece is about temporal data which also happen to use data in a similar way as vector attributes, but for better or worse, they are different implementations, so there are likely differences for testing which would make this into two different sections.
Also, we probably don't want to have here what should be in the documentation (I did not evaluate the state now, but it might be the case here for temporal), but we want to mention things relevant to the tests like testing only the default settings (which uses a SQLite backend, so they don't go into the space of "tests which use databases" in the relational server database sense).
There was a problem hiding this comment.
Separate section now, keeping only what changes the test: created on demand, belongs to the mapset, and the monkeypatch part. Path and tgis.init() detail gone.
Vector attributes and temporal data share per-mapset isolation and the advice to test the default backend, but they are separate implementations, so they get a section each instead of one section about databases. Drop the parts which describe how the temporal database works rather than what a test has to do about it, and say that a map from v.random or v.edit has no attribute table and that the first table defines the connection for the mapset.
|
Split in two, one section each, and dropped the parts describing how the temporal database works rather than what a test needs. Both say to test the default backend. Left the per-map database out since I have not measured it. Can look at that separately if you want the numbers. |
|
Measured it on 20 maps of 200 points. Build 1.01x, read 1.02x, so not slower. It does break v.db.join across maps, since the other table is in a different file. Also only the directory form works, vector/$MAP/sqlite.db. sqlite/$MAP.db fails because v.db.addtable replaces "$MAP/" with the slash, while field.c and pygrass replace a bare $MAP. Can file that separately. |
The guide said data supplied as files is possible only with grass.gunittest. It is not: temporal/t.rast.import/tests reads its data from a directory next to the test, and pyproject.toml already excludes */tests/data/* from collection. Says how to do it instead, and leaves the North Carolina sample dataset as the one reason left to reach for gunittest.
Addresses the database half of #7861. Left the network half out, since I have not written a GRASS test that hits the network.
Describes what is already in the tree: that a vector map brings its attribute table with it, that the temporal database lives in the mapset so a module-scoped session shares one between its tests, that
t.createcreates it, and themonkeypatchmirroringgrass.temporalneeds because it reads the session fromos.environ.Placed before "Tests which run in parallel" to stay clear of #7922, which adds its section after it.