Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use provided dname if available for _noproc #660

Closed
wants to merge 3 commits into from
Closed

use provided dname if available for _noproc #660

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f22146
use provided dname if available
Sep 27, 2022
2c71773
Allow _noproc to run with weaker permissions
Sep 27, 2022
8ba8ff8
replace "postgres" only under --use-database
Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/pytest_postgresql/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def get_config(request: FixtureRequest) -> dict:
"dbname",
"load",
"postgres_options",
"use_database",
]
for option in options:
option_name = "postgresql_" + option
Expand Down
2 changes: 1 addition & 1 deletion src/pytest_postgresql/executor_noop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def version(self) -> Any:
# could be called before self.dbname will be created.
# Use default postgres database
with psycopg.connect(
dbname="postgres",
dbname=self.dbname or "postgres",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but dbname will never be empty 🤔 And we'll attempt to create the database in question.... most probably prior to it's creation 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Indeed, _noproc tries to create a database, and fails. I'd like to be able to use it with a basic database account, but the problem goes deeper than just connecting to the postgres database :-/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With test file test.py:

import pytest
from pytest_postgresql import factories

def test_hello(postgresql_noproc):
    assert True

Run test against an existing database:

# create a basic user
createuser -W pytest  # enter password: pytest
# with a database
createdb -O pytest pytest
# run test
pytest --postgresql-user=pytest --postgresql-password=pytest --postgresql-dbname=pytest --postgresql-use-database test.py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be dependant on the new flag you've added. rather than the self.dbname being null which it will not 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. This means that the option has somehow two effects, but why not.

I've updated the patch.

user=self.user,
host=self.host,
port=self.port,
Expand Down
2 changes: 2 additions & 0 deletions src/pytest_postgresql/factories/noprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ def postgresql_noproc_fixture(request: FixtureRequest) -> Iterator[NoopExecutor]
pg_dbname = xdistify_dbname(dbname or config["dbname"])
pg_options = options or config["options"]
pg_load = load or config["load"]
use_database = config["use_database"]

noop_exec = NoopExecutor(
host=pg_host,
Expand All @@ -90,6 +91,7 @@ def postgresql_noproc_fixture(request: FixtureRequest) -> Iterator[NoopExecutor]
dbname=template_dbname,
version=noop_exec.version,
password=noop_exec.password,
use_database=use_database,
) as janitor:
for load_element in pg_load:
janitor.load(load_element)
Expand Down
12 changes: 11 additions & 1 deletion src/pytest_postgresql/janitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def __init__(
password: Optional[str] = None,
isolation_level: "Optional[psycopg.IsolationLevel]" = None,
connection_timeout: int = 60,
use_database: bool = False,
) -> None:
"""
Initialize janitor.
Expand All @@ -44,13 +45,16 @@ def __init__(
defaults to server's default
:param connection_timeout: how long to retry connection before
raising a TimeoutError
:param use_database: whether to use an existing database
(may imply manual cleanup)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes the janitor effectively using only a loader method that can do something 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay for now, just thinking out loud whether this should be extracted to something separate for this single responsibility later now 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Indeed this parameter makes the janitor out of a job, but I do not think it is worth changing the code structure to avoid it, I'd just let it be.

"""
self.user = user
self.password = password
self.host = host
self.port = port
self.dbname = dbname
self._connection_timeout = connection_timeout
self._use_database = use_database
check_for_psycopg()
self.isolation_level = isolation_level
if not isinstance(version, Version):
Expand All @@ -60,6 +64,8 @@ def __init__(

def init(self) -> None:
"""Create database in postgresql."""
if self._use_database:
return
template_name = f"{self.dbname}_tmpl"
with self.cursor() as cur:
if self.dbname.endswith("_tmpl"):
Expand All @@ -86,13 +92,17 @@ def drop(self) -> None:
"""Drop database in postgresql."""
# We cannot drop the database while there are connections to it, so we
# terminate all connections first while not allowing new connections.
if self._use_database:
return
with self.cursor() as cur:
self._dont_datallowconn(cur, self.dbname)
self._terminate_connection(cur, self.dbname)
cur.execute(f'DROP DATABASE IF EXISTS "{self.dbname}";')

@staticmethod
def _dont_datallowconn(cur: cursor, dbname: str) -> None:
if self._use_database:
return
cur.execute(f'ALTER DATABASE "{dbname}" with allow_connections false;')

@staticmethod
Expand Down Expand Up @@ -136,7 +146,7 @@ def cursor(self) -> Iterator[cursor]:

def connect() -> connection:
return psycopg.connect(
dbname="postgres",
dbname=self.dbname or "postgres",
user=self.user,
password=self.password,
host=self.host,
Expand Down
7 changes: 7 additions & 0 deletions src/pytest_postgresql/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
_help_dbname = "Default database name"
_help_load = "Dotted-style or entrypoint-style path to callable or path to SQL File"
_help_postgres_options = "Postgres executable extra parameters. Passed via the -o option to pg_ctl"
_help_use_database = "Use provided database, do not create/drop it"


def pytest_addoption(parser: Parser) -> None:
Expand Down Expand Up @@ -63,6 +64,7 @@ def pytest_addoption(parser: Parser) -> None:
parser.addini(name="postgresql_unixsocketdir", help=_help_unixsocketdir, default=gettempdir())

parser.addini(name="postgresql_dbname", help=_help_dbname, default="tests")
parser.addini(name="postgresql_use_database", type="bool", help=_help_use_database, default=False)

parser.addini(name="postgresql_load", type="pathlist", help=_help_load, default=None)
parser.addini(name="postgresql_postgres_options", help=_help_postgres_options, default="")
Expand Down Expand Up @@ -119,6 +121,11 @@ def pytest_addoption(parser: Parser) -> None:
"--postgresql-dbname", action="store", dest="postgresql_dbname", help=_help_dbname
)

parser.addoption(
"--postgresql-use-database", action="store_true", dest="postgresql_use_database",
help=_help_use_database
)

parser.addoption("--postgresql-load", action="append", dest="postgresql_load", help=_help_load)

parser.addoption(
Expand Down