feat: Remove/Keep only specified DBs in mongodb_factory#812
Conversation
📝 WalkthroughWalkthroughMongoDB fixture cleanup now accepts ChangesSelective database cleanup
Sequence Diagram(s)sequenceDiagram
participant pytest_addoption
participant get_config
participant MongoConfig
participant mongodb
participant mongo_conn
pytest_addoption->>pytest_addoption: register mongo_remove_dbs and mongo_keep_dbs
get_config->>MongoConfig: build config with remove_dbs and keep_dbs
MongoConfig-->>mongodb: config.remove_dbs and config.keep_dbs
mongodb->>mongodb: validate overlapping remove_dbs and keep_dbs
mongodb->>mongo_conn: list_database_names()
mongo_conn-->>mongodb: database names
mongodb->>mongo_conn: skip keep_dbs and drop selected databases
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pytest_mongo/factories/client.py (2)
12-23: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate the docstring to document the new
databasesparameter.The function signature now includes a
databasesparameter, but the docstring doesn't explain its purpose, expected format, or behaviour. Users of this factory won't know how to use the new parameter.📝 Proposed docstring addition
def mongodb( process_fixture_name: str, tz_aware: bool | None = None, databases: list[str] | None = None, ) -> Callable[[FixtureRequest], Iterator[MongoClient]]: """Mongo database factory. :param str process_fixture_name: name of the process fixture :param bool tz_aware: whether the client to be timezone aware or not + :param list[str] | None databases: list of database names to clean during teardown. + If None, all databases are cleaned (except system.* collections). :rtype: func :returns: function which makes a connection to mongo """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pytest_mongo/factories/client.py` around lines 12 - 23, Update the mongodb factory docstring to document the new databases parameter: explain that databases is an optional list[str] of database names to pre-create or ensure exist for the created MongoClient, describe its expected type (list of database name strings) and default behavior when None (no databases are created/ensured), and note any side effects (e.g., databases will be accessed/created on connect). Place this description alongside the existing parameter docs for process_fixture_name and tz_aware in the mongodb docstring so callers know how to pass and what to expect.
35-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIntegrate the
databasesparameter with plugin configuration.The
databasesparameter should fall back to the configuration value whenNone, following the same pattern astz_aware(lines 36-39). Currently, the--mongo-databasesCLI option andmongo_databasesini option defined inplugin.pyhave no effect because the factory never reads them.🔧 Proposed fix to wire up configuration
config = get_config(request) mongo_tz_aware = False if tz_aware is not None: mongo_tz_aware = tz_aware elif config.tz_aware is not None and isinstance(config.tz_aware, bool): mongo_tz_aware = config.tz_aware + + mongo_databases = databases + if mongo_databases is None and hasattr(config, 'mongo_databases'): + mongo_databases = config.mongo_databasesThen update line 49 to use
mongo_databasesinstead ofdatabases:for db_name in mongo_conn.list_database_names(): - if databases and db_name not in databases: + if mongo_databases and db_name not in mongo_databases: continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pytest_mongo/factories/client.py` around lines 35 - 39, The factory currently ignores the plugin config for databases; mirror the tz_aware fallback logic: when the function parameter databases is None, set a local mongo_databases from config.mongo_databases (and only use it if it's a list/expected type) otherwise use the provided databases value. Update the creation call that currently passes databases to instead pass the resolved mongo_databases variable (same place where tz_aware becomes mongo_tz_aware), referencing the parameter name databases, the config attribute mongo_databases, and the resolved local variable mongo_databases.
🧹 Nitpick comments (1)
tests/test_mongo.py (1)
74-77: 💤 Low valueConsider making the test more resilient to execution order.
The second test method depends on the first test running successfully and seeding
test_db2. If the first test fails or is skipped, this test will also fail. Consider adding a setup fixture or combining both assertions into a single test to make the test suite more robust.♻️ Alternative approach: combine into a single test
- def test_clean_specified_databases(self, mongodb4: MongoClient) -> None: - """Test if only specified databases are cleaned.""" + def test_clean_specified_databases_first_run(self, mongodb4: MongoClient) -> None: + """Seed databases for cleanup test.""" test_db = mongodb4["test_db"] test_db.test.insert_one({"test": "test"}) test_db2 = mongodb4["test_db2"] test_db2.test.insert_one({"test": "test"}) assert "test_db" in mongodb4.list_database_names() assert "test_db2" in mongodb4.list_database_names() - def test_clean_specified_databases_again(self, mongodb4: MongoClient) -> None: - """Test if only specified databases are cleaned.""" + def test_clean_specified_databases_second_run(self, mongodb4: MongoClient) -> None: + """Verify selective cleanup removed only test_db.""" + # Re-seed test_db2 if it doesn't exist (makes test independent) + if "test_db2" not in mongodb4.list_database_names(): + test_db2 = mongodb4["test_db2"] + test_db2.test.insert_one({"test": "test"}) + assert "test_db" not in mongodb4.list_database_names() assert "test_db2" in mongodb4.list_database_names()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_mongo.py` around lines 74 - 77, The test test_clean_specified_databases_again currently assumes prior test seeded "test_db2" causing order-dependent failures; make it resilient by ensuring the DB exists before asserting: either add a setup fixture (e.g., a pytest fixture that uses the MongoClient parameter mongodb4 to create/seed "test_db2" via create_collection or insert_one and yield the client) and use that fixture in test_clean_specified_databases_again, or combine this assertion with the earlier test into one consolidated test that both seeds and verifies presence/absence of "test_db" and "test_db2"; reference test_clean_specified_databases_again and the mongodb4/list_database_names calls when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_mongo/plugin.py`:
- Around line 34-37: The help text in the _help_databases constant contains a
grammar error ("exclude" should be "excluding"); update the string assigned to
_help_databases so it reads: "List of MongoDB databases to clean in the fixture.
Otherwise, all databases are cleaned, excluding system.*." Keep the same
variable name (_help_databases) and only change the word and punctuation to
produce a grammatically correct, user-facing message.
---
Outside diff comments:
In `@pytest_mongo/factories/client.py`:
- Around line 12-23: Update the mongodb factory docstring to document the new
databases parameter: explain that databases is an optional list[str] of database
names to pre-create or ensure exist for the created MongoClient, describe its
expected type (list of database name strings) and default behavior when None (no
databases are created/ensured), and note any side effects (e.g., databases will
be accessed/created on connect). Place this description alongside the existing
parameter docs for process_fixture_name and tz_aware in the mongodb docstring so
callers know how to pass and what to expect.
- Around line 35-39: The factory currently ignores the plugin config for
databases; mirror the tz_aware fallback logic: when the function parameter
databases is None, set a local mongo_databases from config.mongo_databases (and
only use it if it's a list/expected type) otherwise use the provided databases
value. Update the creation call that currently passes databases to instead pass
the resolved mongo_databases variable (same place where tz_aware becomes
mongo_tz_aware), referencing the parameter name databases, the config attribute
mongo_databases, and the resolved local variable mongo_databases.
---
Nitpick comments:
In `@tests/test_mongo.py`:
- Around line 74-77: The test test_clean_specified_databases_again currently
assumes prior test seeded "test_db2" causing order-dependent failures; make it
resilient by ensuring the DB exists before asserting: either add a setup fixture
(e.g., a pytest fixture that uses the MongoClient parameter mongodb4 to
create/seed "test_db2" via create_collection or insert_one and yield the client)
and use that fixture in test_clean_specified_databases_again, or combine this
assertion with the earlier test into one consolidated test that both seeds and
verifies presence/absence of "test_db" and "test_db2"; reference
test_clean_specified_databases_again and the mongodb4/list_database_names calls
when applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbc4b766-8027-4038-a965-7a711332fc55
📒 Files selected for processing (4)
pytest_mongo/factories/client.pypytest_mongo/plugin.pytests/conftest.pytests/test_mongo.py
|
hey @fizyk, could you take a look at the PR, please? |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pytest_mongo/config.py (1)
25-27:⚠️ Potential issue | 🟠 MajorNormalise
mongo_databasesto a list before storing inMongoConfig.Line 38 currently forwards raw
getoption/getinioutput, butMongoConfig.databasesis typed aslist[str] | None(line 19) and downstream filtering only activates forlist. The ini registration (plugin.py line 65–68) omits an explicittypeparameter, so if a user configuresmongo_databasesdirectly in pytest.ini (rather than via--mongo-databases), the value may not be parsed as a list. A non-list value will silently disable database filtering.Suggested fix
def get_config(request: FixtureRequest) -> MongoConfig: """Return a MongoConfig with config options.""" @@ port = get_mongo_option("port") + databases = get_mongo_option("databases") + if isinstance(databases, str): + databases = [db for db in databases.split() if db] + elif databases is not None and not isinstance(databases, list): + databases = list(databases) @@ params=get_mongo_option("params"), tz_aware=get_mongo_option("tz_aware"), - databases=get_mongo_option("databases"), + databases=databases or None, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pytest_mongo/config.py` around lines 25 - 27, The `mongo_databases` configuration may arrive as a non-list value when read from pytest.ini (due to missing type specification in the ini registration), but `MongoConfig.databases` expects `list[str] | None`. Normalize the `mongo_databases` value to a list before assigning it to `MongoConfig.databases` at line 38. Add logic to check if the returned value from `get_mongo_option` is a string and convert it to a list (by splitting on common delimiters like commas), or wrap it in a list if it's a single string, ensuring the value is always `None` or a proper list when stored in the config object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pytest_mongo/config.py`:
- Around line 25-27: The `mongo_databases` configuration may arrive as a
non-list value when read from pytest.ini (due to missing type specification in
the ini registration), but `MongoConfig.databases` expects `list[str] | None`.
Normalize the `mongo_databases` value to a list before assigning it to
`MongoConfig.databases` at line 38. Add logic to check if the returned value
from `get_mongo_option` is a string and convert it to a list (by splitting on
common delimiters like commas), or wrap it in a list if it's a single string,
ensuring the value is always `None` or a proper list when stored in the config
object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 128febc1-a75a-46aa-adc3-095cd7a31389
📒 Files selected for processing (4)
newsfragments/812.feature.rstpytest_mongo/config.pypytest_mongo/factories/client.pypytest_mongo/plugin.py
✅ Files skipped from review due to trivial changes (1)
- newsfragments/812.feature.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- pytest_mongo/factories/client.py
- pytest_mongo/plugin.py
fizyk
left a comment
fizyk
left a comment
There was a problem hiding this comment.
@tyzhnenko
I wonder if we shouldn't also create said database in the beginning.
It wouldn't necessarily change the behavbiour for existing tests, but the fixtures could come with pre-configured database already, and that'd be the database we'd drop. the name would have to be xdistified of course. becuase as you have problems on xdist, you needt o make sure that for no-process fixture, you'd create unique databases on the docker instance (or running server)
And.... we need to make sure we don't attempt to create database twice. Mongo creates dbs automatically, right? (haven't used it for a long time) So we'd need to make sure we don't overuse some names in different sessions.
| assert server_info["ok"] == 1.0 | ||
|
|
||
|
|
||
| class TestCleanSpecifiedDatabases: |
There was a problem hiding this comment.
please rewrite this to classic function-style pytest tests. You can safely separate that to it's own module.
|
Hey @fizyk. You're right, we can't control which DB would be created by a driver. It does creation automatically. If some DB is already there with some pre-populated data and we want to keep it, we just need to specify it in the new parameter (opt-out). The logic of the fixture is to clean up the database (remove all collections) after each test. So, we can have two parameters. The first one with an opt-in list of databases. The second one is with an out-out list (as I've already implemented). What do you think? We just need to figure out the priority in case a user specifies both. On the other hand we can forbid using both parameters, it's much simpler.
|
|
@tyzhnenko current solution is fine as a stopgap, but the more elegant one would be to actually define the dbnames for every client (or even process?) that way we'd know what to manage from start to finish without the divergence to sometimes delete defined database and sometimes all of them. |
@fizyk Yeah, I see your point. I'll check the PR and do some updates. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pytest_mongo/plugin.py (1)
34-41: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueMinor grammar in help text.
Both help strings end with
excluding system.*. collections, which reads awkwardly (misplaced period). Suggestexcluding system.* collections..🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pytest_mongo/plugin.py` around lines 34 - 41, The help text strings in the plugin module have awkward punctuation at the end of the “excluding system.*. collections” phrase. Update both _help_remove_dbs and _help_keep_dbs so the sentence reads naturally as “excluding system.* collections.” while keeping the rest of the wording unchanged.
🧹 Nitpick comments (1)
tests/test_mongo_cleanup.py (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrder-dependent and duplicated tests reduce reliability.
test_clean_specified_databases_againandtest_keep_specified_databasescontain identical assertions, and both rely ontest_clean_specified_databaseshaving run first to createtest_db/test_db2. Running either in isolation (e.g.pytest -k test_keep) or under reordering/xdist would fail because the expected state would not have been seeded. Consider seeding the required state within each test (or a shared fixture) and removing the duplicate so each test is self-contained.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_mongo_cleanup.py` around lines 15 - 23, `test_clean_specified_databases_again` and `test_keep_specified_databases` are duplicate, order-dependent checks that rely on `test_clean_specified_databases` to seed `test_db` and `test_db2`. Update these tests to make each one self-contained by creating the required database state in a shared fixture or inside each test before asserting against `mongodb5.list_database_names()`, and remove the redundant duplicate assertion block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_mongo/plugin.py`:
- Around line 68-78: The mongo_remove_dbs and mongo_keep_dbs ini options in
plugin.py are currently registered without an explicit type, so pytest treats
them as strings instead of list values. Update the parser.addini calls for
mongo_remove_dbs and mongo_keep_dbs to use the appropriate list-oriented type
(such as args, or linelist if that is the intended format) while keeping
default=[] so their values are handled consistently with the list-based logic in
client.py.
---
Duplicate comments:
In `@pytest_mongo/plugin.py`:
- Around line 34-41: The help text strings in the plugin module have awkward
punctuation at the end of the “excluding system.*. collections” phrase. Update
both _help_remove_dbs and _help_keep_dbs so the sentence reads naturally as
“excluding system.* collections.” while keeping the rest of the wording
unchanged.
---
Nitpick comments:
In `@tests/test_mongo_cleanup.py`:
- Around line 15-23: `test_clean_specified_databases_again` and
`test_keep_specified_databases` are duplicate, order-dependent checks that rely
on `test_clean_specified_databases` to seed `test_db` and `test_db2`. Update
these tests to make each one self-contained by creating the required database
state in a shared fixture or inside each test before asserting against
`mongodb5.list_database_names()`, and remove the redundant duplicate assertion
block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a4c5f71-c39b-489b-b8a1-0649da228ee7
📒 Files selected for processing (5)
pytest_mongo/config.pypytest_mongo/factories/client.pypytest_mongo/plugin.pytests/conftest.pytests/test_mongo_cleanup.py
|
I've brought some updates. @fizyk take a look please Now we have keep and remove only lists. I put it to client fixture due to it looks more flexible. Only one server can be used to run tests parallel. It believe this can save some costs for CICD infrastructure. |
tyzhnenko
changed the title
2 participants
Chore that needs to be done:
pipenv run towncrier create [issue_number].[type].rstTypes are defined in the pyproject.toml, issue_numer either from issue tracker or the Pull request number
Summary by CodeRabbit
New Features
Bug Fixes
Tests