[FIX] common: only report an available update when there is one - #175
Closed
brinkflew wants to merge 1 commit into
Closed
[FIX] common: only report an available update when there is one#175brinkflew wants to merge 1 commit into
brinkflew wants to merge 1 commit into
Conversation
sea-odoo
previously approved these changes
Jul 30, 2026
brinkflew
added a commit
that referenced
this pull request
Aug 6, 2026
odev.common.logging configures logging on import, but logging.basicConfig is a no-op once the root logger has handlers: whether odev's handler gets installed depends on whether that import happens before or after pytest sets its own up, and importing the sandbox from conftest tips it. Combined with the capture added by #176 every record then reached the output twice, and a plain logger.info became a console.print that #177's run_hook test asserts on. The handler is dropped in pytest_configure so the suite no longer depends on import order. Also resolves the version command test, which this branch made namespace-agnostic while #175 rewrote it, and bumps the version to 4.31.2, one increment above the base branch.
6 tasks
Contributor
Author
Merge order: 2nd of 4Important Merge #174 first. The base of this PR is now
What the merge commit resolves#174 and this branch both added tests numbered
Queue
Warning GitHub Actions has not run in this repository since 2026-07-29, so the checks here are stale. The full suite was run locally against the exact merged tree of all four: 357 passed, pre-commit clean. |
6 tasks
sea-odoo
previously approved these changes
Aug 27, 2026
…st suite (#178) * [FIX] common: correct string, version and postgres helpers Writing unit tests for helpers that had none surfaced five defects, all on code paths odev actually walks: - `PostgresConnector.columns_exist` returned an empty list when none of the requested columns existed, which is indistinguishable from all of them being present. A table created from an older definition therefore kept none of its new columns, since `CREATE TABLE IF NOT EXISTS` leaves an existing table alone and the missing-columns pass is the only thing that can migrate it. - `PostgresDatabase.tables` was a class attribute, so every database instance shared a single registry and tables from different databases collided on their name alone. - `string.quote` selected its delimiter with `max()` over the offsets of both quote characters, which picks the last one rather than the first and mis-quoted any string mixing them. The helper never escapes, so its docstring now says so. - `OdooVersion.__bool__` was always true: `module` is padded to `MIN_VERSION_LENGTH` and is therefore never an empty tuple. - `string.min_indent` raised on a text without any non-blank line, which `odev help` reaches through `dedent`. `float_to_hours` and `strip_styles` are broken too but are called nowhere in odev nor in the plugins; they are left alone and documented in the tests instead. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du * [IMP] tests: cover string, worktree, version and postgres helpers The suite sat at 64% with `.coveragerc` gating at 60%, and the gap was widest on pure logic the rest of the framework leans on. `odev/common/string.py` had no test module at all despite backing `odev help`, `odev history` and the database listing query. `GitWorktree` turns `git worktree list --porcelain` into the objects the whole fetch/pull/worktree family works with, and nothing exercised it. `OdooVersion` was tested for parsing only, while ordering is what actually picks a revision at runtime. Add test modules for the string helpers, the worktree parser and the datastore table preparation, plus ordering tests for versions. None of them need a network or a real repository. String and version helpers reach 100%, `common/postgres.py` 81% to 93% and the git connector 34% to 40%. Correct four things in the existing suite along the way: - The sudo tests shelled out to a real `sudo cat >> /etc/shadow`. The premise that the command fails only holds for an unprivileged user whose shell cannot open the redirection; a machine granting passwordless sudo runs it for real. Simulate the subprocess and the effective user instead, which also lets the elevation path be asserted rather than inferred. - `test_odev` left a command line behind in `sys.argv` for whichever test ran next. - `_patches` was a list defined on `OdevTestCase`, shared by every subclass through `cls._patches.append`, so each class tore down the patches of all the classes before it. - `PostgresTable` preparation is asserted against a mocked database: driving it against the live datastore made it depend on the connector's query cache, which DDL does not invalidate. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du * [FIX] common: keep listing databases when one is dropped mid-inspection `is_odoo` checks that a database exists and then connects to it, and any process can drop it in between: `odev list` inspects every database in turn and would fail outright because one went away. A database that is gone is not an Odoo database, while anything else stays an error. The second check bypasses the query cache, since the cache is what claimed the database was still there. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du * [IMP] common: let the framework namespace and the command link be chosen `Odev` derived its name from the test mode alone, and everything it owns is named after it: the configuration file and the datastore database were fixed paths that any two instances had to share. It can now be given a name of its own. The setup script computed the destination of the `odev` symlink halfway through creating it, leaving no way to point it elsewhere; the decision moves to `link_path`. Both make the test suite able to run against resources of its own rather than against those of the user. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du * [IMP] tests: run each suite in a sandbox of its own and clean it up Everything the suite touched was named after a constant `odev-test`: the datastore database, the configuration file, the run directories and the databases created by the command tests. Two suites running at once shared all of it and destroyed each other's work — `odev delete --expression` removed the databases of the other run, dropping the datastore terminated its connections, and both wrote the same configuration file at once. A run now claims a sandbox named after itself and holds an exclusive lock on it for its whole life. The sandbox is removed when the session ends, including on `Ctrl+C` and on `SIGTERM`; the kernel drops the lock however the process dies, so a lock that can be taken marks leftovers the next run sweeps away. Nothing is written outside of it anymore: the configuration directory, the repositories and dumps directories, and the two symlinks the setup scripts create all point inside the sandbox, which also keeps the suite from repointing the `odev` command of the developer at whichever checkout it happens to run from. odev captures `SIGINT` around every query to cancel it rather than let it through, so an interrupt is recorded and acted upon at the next test boundary: pressing `Ctrl+C` stops the run without abandoning a connection mid-statement. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du * [FIX] common: close the PostgreSQL connection a database block opened Both database context managers built a second, unconnected connector to close instead of the one they had connected, so `disconnect` did nothing and the connection stayed open until the garbage collector got to it. `ensure_connected` runs every database method inside its own block and those blocks nest — `is_odoo` opens one and then calls `table_exists`, which opens another — so this meant a fresh backend per call. Closing the right connector is not enough on its own: an inner block would close the connection the enclosing one is still using. The blocks are now reentrant and share a single connector, counted in the mixin so both classes get the same behaviour. The datastore keeps its connection instead of reopening it for each read: every command reads it and it lives as long as the process, which is not true of the databases odev walks through for `list` or `delete`. Over a full test suite run, the backends held at once drop from 42 to 3, and the suite goes from 55s to 34s. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du * [IMP] tests: cover the lifetime of database connections Pin the behaviour a connection block is expected to have, since nothing failed loudly when it did not have it: leaving a block closes what it opened, a nested block joins the connection of the enclosing one rather than opening its own, repeated calls to decorated methods do not leave backends behind, and the datastore holds a single one throughout. The count is read from `pg_stat_activity`, which is what makes the third one a regression guard rather than a restatement of the code. Claude-Session: https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du
Contributor
Author
|
Merged |
brinkflew
added a commit
that referenced
this pull request
Aug 27, 2026
## Description `odev version` warned "A newer version is available, consider running 'odev update'" on an up-to-date checkout, and `odev update` could not make the warning go away: ``` odev version [i] Odev version 4.30.0 (main) [!] A newer version is available, consider running 'odev update' odev update [i] Current version: 4.30.0 [i] Odev is up to date odev version [i] Odev version 4.30.0 (main) [!] A newer version is available, consider running 'odev update' ``` The check never looked at the remote. It compared `config.update.version` — the marker recording which version the upgrade scripts last ran for — against `_version.py`, with `!=`, so it fired in both directions, and the command *displayed* that marker instead of the version actually running. Any drift (typically switching back from the `beta` release channel) made the warning stick forever, since `upgrade()` only rewrites the marker when the code version is higher. That drift also silently suppresses the upgrade script of the version it is stuck on. The version command now reports the running version and warns based on the remote tracking branch, through a new `Odev.update_available()` that reuses `__git_branch_behind()` and the ref already fetched by the periodic check — no network call and nothing added to the startup path. Along the same lines in the self-update flow, which behaved as if an update existed when it did not: - Restore the early return dropped in fea554e: the result of `__git_branch_behind()` was discarded, so every check went on to prompt (`mode = ask`) and pull (`mode = always`) even when up to date. The check now runs again after fetching, so a commit arriving in that very fetch is still pulled in the same run. - Record `update.date` whenever a check ran instead of only when something was pulled, as its name implies and as returning early now requires — otherwise every single command would fetch again. - Reset a recorded version that is ahead of the running one, from `upgrade()` and when switching release channel, so the marker cannot drift. - Bail out of the behind check on a detached HEAD instead of accessing the active branch, which raises. Verified with the full suite (10 new tests covering the behind/ahead/up-to-date matrix, detached HEAD, the early return, the date stamping and the marker reset), and end to end against a throwaway local repository pair: up to date fetches only and pulls nothing, remote ahead pulls in the same run. ## Linked Issues None. ## Compliance - [x] I have read the [contribution guide](../docs/CONTRIBUTING.md) - [x] I made sure the documentation is up-to-date both in doctrings and the `docs` directory - [x] I have added or modified unit tests where necessary - [x] I have added new libraries to the `requirements.txt` file, if any (none added) - [x] I have incremented the version number according the [versioning guide](../../docs/contributing/versioning.md) - [x] The PR contains **my changes only** and **no other external commit** 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01ADsnaLfNDoobrG9bf8NfmV
brinkflew
added a commit
that referenced
this pull request
Aug 27, 2026
…st suite (#178) ## Why This started as filling coverage gaps, and each step surfaced the next: 1. `.coveragerc` gates at 60% and the suite sat at **64%**, with the gap widest on pure logic the rest of the framework leans on. Writing those tests surfaced **six defects in the helpers** — without fixing them the tests would have pinned broken behaviour. 2. Verifying the fixes meant running the suite repeatedly, which is when it became clear that **two suites cannot run at once**: identical invocations produced anywhere from 0 to 68 failures, and the machine had accumulated 16 orphaned `/tmp/odev-test-*` directories. 3. Isolating the runs revealed that three concurrent suites exhaust PostgreSQL's connection slots, which turned out to be a **connection leak in odev itself**, not in the suite. The four sections below are independent and the commits are ordered to be reviewed in sequence. ## 1. Helper defects — `42569c4` | Location | Defect | |---|---| | `connectors/postgres.py` `columns_exist` | Returned `[]` when **none** of the requested columns existed — indistinguishable from all being present. `CREATE TABLE IF NOT EXISTS` leaves an existing table alone, so the missing-columns pass is the only thing that can migrate a table created from an older definition; it silently added nothing. | | `postgres.py` `PostgresDatabase.tables` | A class attribute, so every instance shared one registry and tables from different databases collided on their name alone. | | `string.py` `quote` | Chose its delimiter with `max()` over both quote offsets, picking the **last** rather than the first, mis-quoting any string mixing them. | | `version.py` `OdooVersion.__bool__` | Always `True` — `module` is padded to `MIN_VERSION_LENGTH` and is never an empty tuple. | | `string.py` `min_indent` | Raised `ValueError` on a text without any non-blank line, reachable from `odev help` through `dedent`. | | `float_to_hours`, `strip_styles` | Broken, but called nowhere in odev nor in the plugins. **Left alone**, documented in the tests with the correction spelled out. | `columns_exist` has exactly one caller, and it runs after `CREATE TABLE IF NOT EXISTS`, so the fix cannot make it issue `ALTER TABLE` against a missing table. ## 2. Coverage for the untested helpers — `be95197` - **`test_string.py`** (new) — `string.py` had no test module at all, despite backing `odev help`, `odev history` and the local database listing query. Sizes and their round-trip, indentation, joining, the `dirty_only` × `force_single` quoting matrix, Rich markup helpers, and the `help` column alignment contract. - **`test_git_worktree.py`** (new) — `connectors/git.py` was the least-covered large module (34%), and its `GitWorktree` parser turns `git worktree list --porcelain` into the objects the whole `fetch` / `pull` / `worktree` family works with. Porcelain parsing (branch, detached, bare, locked, prunable with reasons), the `-odev-` local-branch split that `create_worktree` writes and `fetch` / `pull` read back, identity by path, and `pending_changes` including the two swallowed `GitCommandError` messages. No network, no real repository. - **`test_postgres_table.py`** (new) — `PostgresTable.__add_missing_column`, the datastore's migration path, was entirely unreached; this covers it including the `InvalidTableDefinition` primary-key branch. - **`test_version.py`** — ordering (`15.0 < 16.0 < saas-16.4 < 17.0 < master`) is what actually picks a revision at runtime and nothing compared two versions. Corrections to existing tests, in the same commit: - **`test_bash.py` shelled out to a real `sudo cat >> /etc/shadow`.** The premise that the command fails only holds for an unprivileged user whose shell cannot open the redirection — a machine granting passwordless sudo runs it for real, and as root it appends to the file or hangs on stdin. The subprocess and the effective user are now simulated, which also lets the elevation path be asserted rather than inferred. - `test_odev.py` left a command line behind in `sys.argv` for whichever test ran next. - `tests/fixtures/case.py` — `_patches` was a list defined on `OdevTestCase` and mutated through `cls._patches.append`, so every subclass shared it and each class tore down the patches of all the classes before it. ## 3. An isolated, self-cleaning test suite — `87dc0e4`, `86630d3`, `716c8e1` `Odev.name` was the constant `"odev-test"` and **every** shared resource derived from it, so two suites shared one namespace and actively destroyed each other: - `test_99_delete_expression` ran `odev delete --expression "^odev-test-[a-z0-9]{8}" --include-whitelisted` against the real PostgreSQL, deleting a concurrent run's databases. - `PostgresDatabase.drop()` terminates every backend on `datname`, so each class teardown killed a concurrent run's cursors. - `CREATE TABLE IF NOT EXISTS` is not atomic, and `Config.save()` truncate-writes a fixed path — hence `UniqueViolation` on `pg_type_typname_nsp_index` and `DuplicateOptionError` from a torn config. A run now claims a sandbox named after itself and holds an exclusive `flock` on it for its whole life. Everything — datastore, test databases, config, temp directories — is named after it or nested under it. Cleanup runs at `pytest_sessionfinish`, which pytest calls from a `finally`, so `Ctrl+C` is covered; `SIGTERM` becomes the same orderly exit; and the next run's sweep collects whatever a `SIGKILL` left, because the kernel releases the lock when the owner dies whatever the cause. **The suite was also writing outside its sandbox**, which is worth a look on its own: - `TestSetup` ran the install scripts against their real destinations, so running the suite **repointed the developer's `~/.local/bin/odev` and bash-completion symlinks at whichever checkout it ran from**. `symlink.py` computed the destination halfway through creating it, leaving no way to redirect it; that decision moves to `link_path`. - Tests cloned into the real `~/odoo/repositories`. The repositories, dumps and upgrade paths now point inside the sandbox, as does `CONFIG_DIR` — which also means the suite no longer picks up whichever plugins the developer happens to have installed, so a local run and CI exercise the same code. `87dc0e4` is a separate product fix this surfaced: `LocalDatabase.is_odoo` checks that a database exists and then connects to it, and any process can drop it in between — `odev list` inspects every database in turn and would fail outright because one went away. Interrupt handling is covered by `tests/tests/common/test_interrupts.py`: odev captures `SIGINT` around every query to cancel just that statement, so a `Ctrl+C` was previously swallowed and the run carried on. Letting it through instead abandons the connection mid-statement, so the interrupt is recorded and acted upon at the next test boundary. ## 4. Connection lifetime — `47499cb`, `cd07007` Both database context managers built a **second, unconnected** connector to close instead of the one they had connected, so `disconnect()` did nothing and the connection stayed open until the garbage collector got to it: ```python def __enter__(self): self.connector = self._connector_class(self.name).__enter__() # connector A, connected return self def __exit__(self, *args): self._connector_class(self.name).__exit__(*args) # connector B, never connected ``` `ensure_connected` runs every database method inside its own block and those blocks nest — `is_odoo` opens one and then calls `table_exists`, which opens another — so this meant a fresh backend per call. Closing the right connector is **not enough on its own**: an inner block would close the connection the enclosing one is still using. The blocks are now reentrant and share a single connector, counted in `PostgresConnectorMixin` so both classes get the same behaviour. The datastore holds its connection instead of reopening it per read — every command reads it and it lives as long as the process, which is not true of the databases odev walks through for `list` or `delete`. A connection pool keyed per database was considered and set aside: `list --all` and `delete --expression` touch **every** database on the server briefly, so a per-database pool would hold one idle backend per Odoo database until the process ends — the very exhaustion this fixes — unless it also grew a global cap and idle eviction. Measured over a full suite run: | | before | after | |---|---|---| | peak backends held | 42 | **3** | | mean backends held | 8.7 | **0.8** | | suite duration | 54.8s | **33.6s** | | three concurrent suites | died on `max_connections` | **249 passed each**, peak 7 backends | The speedup was not the goal — it is what a backend fork plus an authentication round-trip per query costs. ## Coverage | Module | Before | After | |---|---|---| | `common/string.py` | 85% | **100%** | | `common/version.py` | 96% | **100%** | | `common/postgres.py` | 81% | **93%** | | `common/connectors/git.py` | 34% | **40%** | | **Total** | **64%** | **65%** | ## Verification - `pytest tests` — **249 passed**, from 242 on the first revision - Two and three concurrent suites — **249 passed each**, repeatedly, leaving zero directories and zero databases behind - `SIGINT`, `SIGTERM` and `SIGKILL` mid-run — each verified to leave nothing behind, the last one via the next run's sweep - `odev list --all`, `odev history`, `odev version` — smoke-checked, no connections surviving the process - `pre-commit run --all-files` — clean - `basedpyright` — 3 errors, all pre-existing on `beta`; **0 new** ## Notes for reviewers - `odev/_version.py` is bumped once, to `4.29.10`. `origin/beta` is at `4.29.9`; PRs #175, #176 and #177 each bump from the same base, so whichever merges second needs a one-line rebase. - `LocalDatabase.connector: PostgresConnector | None = None` was removed as dead — `ConnectorMixin.__init__` overwrites it with the connector *class* at construction, which also meant the `if self.connector is not None` guard in `_restore` never protected anything. It is now the `isinstance` check `drop()` already used. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
odev versionwarned "A newer version is available, consider running 'odev update'" on an up-to-date checkout, andodev updatecould not make the warning go away:The check never looked at the remote. It compared
config.update.version— the marker recording which version the upgrade scripts last ran for — against_version.py, with!=, so it fired in both directions, and the command displayed that marker instead of the version actually running. Any drift (typically switching back from thebetarelease channel) made the warning stick forever, sinceupgrade()only rewrites the marker when the code version is higher. That drift also silently suppresses the upgrade script of the version it is stuck on.The version command now reports the running version and warns based on the remote tracking branch, through a new
Odev.update_available()that reuses__git_branch_behind()and the ref already fetched by the periodic check — no network call and nothing added to the startup path.Along the same lines in the self-update flow, which behaved as if an update existed when it did not:
__git_branch_behind()was discarded, so every check went on to prompt (mode = ask) and pull (mode = always) even when up to date. The check now runs again after fetching, so a commit arriving in that very fetch is still pulled in the same run.update.datewhenever a check ran instead of only when something was pulled, as its name implies and as returning early now requires — otherwise every single command would fetch again.upgrade()and when switching release channel, so the marker cannot drift.Verified with the full suite (10 new tests covering the behind/ahead/up-to-date matrix, detached HEAD, the early return, the date stamping and the marker reset), and end to end against a throwaway local repository pair: up to date fetches only and pulls nothing, remote ahead pulls in the same run.
Linked Issues
None.
Compliance
docsdirectoryrequirements.txtfile, if any (none added)🤖 Generated with Claude Code
https://claude.ai/code/session_01ADsnaLfNDoobrG9bf8NfmV