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.

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

set local session posargs via notify #397

Merged
merged 7 commits into from
Mar 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 21 additions & 2 deletions nox/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,18 @@
import collections.abc
import itertools
from collections import OrderedDict
from typing import Any, Iterable, Iterator, List, Mapping, Sequence, Set, Tuple, Union
from typing import (
Any,
Iterable,
Iterator,
List,
Mapping,
Optional,
Sequence,
Set,
Tuple,
Union,
)

from nox._decorators import Call, Func
from nox.sessions import Session, SessionRunner
Expand Down Expand Up @@ -257,7 +268,9 @@ def make_session(
def next(self) -> SessionRunner:
return self.__next__()

def notify(self, session: Union[str, SessionRunner]) -> bool:
def notify(
self, session: Union[str, SessionRunner], posargs: Optional[List[str]] = None
rmorshea marked this conversation as resolved.
Show resolved Hide resolved
rmorshea marked this conversation as resolved.
Show resolved Hide resolved
) -> bool:
"""Enqueue the specified session in the queue.

If the session is already in the queue, or has been run already,
Expand All @@ -266,6 +279,10 @@ def notify(self, session: Union[str, SessionRunner]) -> bool:
Args:
session (Union[str, ~nox.session.Session]): The session to be
enqueued.
posargs (Optional[List[str]]): If given, sets the positional
arguments *only* for the queued session. Otherwise, the
standard globally available positional arguments will be
used instead.

Returns:
bool: Whether the session was added to the queue.
Expand All @@ -282,6 +299,8 @@ def notify(self, session: Union[str, SessionRunner]) -> bool:
# the end of the queue.
for s in self._all_sessions:
if s == session or s.name == session or session in s.signatures:
if posargs is not None:
s.posargs = posargs
rmorshea marked this conversation as resolved.
Show resolved Hide resolved
self._queue.append(s)
return True

Expand Down
22 changes: 16 additions & 6 deletions nox/sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,8 @@ def env(self) -> dict:

@property
def posargs(self) -> List[str]:
"""This is set to any extra arguments
passed to ``nox`` on the commandline."""
return self._runner.global_config.posargs
"""Any extra arguments from the ``nox`` commandline or :class:`Session.notify`."""
return self._runner.posargs

@property
def virtualenv(self) -> ProcessEnv:
Expand Down Expand Up @@ -432,7 +431,11 @@ def install(self, *args: str, **kwargs: Any) -> None:

self._run("python", "-m", "pip", "install", *args, external="error", **kwargs)

def notify(self, target: "Union[str, SessionRunner]") -> None:
def notify(
self,
target: "Union[str, SessionRunner]",
posargs: Optional[Iterable[str]] = None,
) -> None:
"""Place the given session at the end of the queue.

This method is idempotent; multiple notifications to the same session
Expand All @@ -442,8 +445,14 @@ def notify(self, target: "Union[str, SessionRunner]") -> None:
target (Union[str, Callable]): The session to be notified. This
may be specified as the appropriate string (same as used for
``nox -s``) or using the function object.
posargs (Optional[Iterable[str]]): If given, sets the positional
arguments *only* for the queued session. Otherwise, the
standard globally available positional arguments will be
used instead.
"""
self._runner.manifest.notify(target)
if posargs is not None:
posargs = list(posargs)
self._runner.manifest.notify(target, posargs)

def log(self, *args: Any, **kwargs: Any) -> None:
"""Outputs a log during the session."""
Expand Down Expand Up @@ -472,7 +481,8 @@ def __init__(
self.func = func
self.global_config = global_config
self.manifest = manifest
self.venv = None # type: Optional[ProcessEnv]
self.venv: Optional[ProcessEnv] = None
self.posargs: List[str] = global_config.posargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we inoculate global_config.posargs against mutation here?

Suggested change
self.posargs: List[str] = global_config.posargs
self.posargs: List[str] = global_config.posargs[:]

Now that every session conceptually has their own posargs, I think it makes sense to prevent one session from accidentally mutating the default posargs of other sessions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh dear, this breaks a whole lot of tests 😅

Essentially, there are three groups of tests that break:

  • Tests that compare posargs using is instead of ==
  • Tests that construct a namespace manually without including posargs
  • Tests that construct a namespace manually and set posargs to a mock sentinel
Here's a patch that would make this green again.

(Some of these could be turned into fixtures.)

diff --git a/tests/test__option_set.py b/tests/test__option_set.py
index 3ad8b59..d05bd7f 100644
--- a/tests/test__option_set.py
+++ b/tests/test__option_set.py
@@ -55,7 +55,7 @@ class TestOptionSet:
             optionset.namespace(non_existant_option="meep")
 
     def test_session_completer(self):
-        parsed_args = _options.options.namespace(sessions=(), keywords=())
+        parsed_args = _options.options.namespace(sessions=(), keywords=(), posargs=[])
         all_nox_sessions = _options._session_completer(
             prefix=None, parsed_args=parsed_args
         )
@@ -65,7 +65,9 @@ class TestOptionSet:
         assert len(set(some_expected_sessions) - set(all_nox_sessions)) == 0
 
     def test_session_completer_invalid_sessions(self):
-        parsed_args = _options.options.namespace(sessions=("baz",), keywords=())
+        parsed_args = _options.options.namespace(
+            sessions=("baz",), keywords=(), posargs=[]
+        )
         all_nox_sessions = _options._session_completer(
             prefix=None, parsed_args=parsed_args
         )
diff --git a/tests/test_manifest.py b/tests/test_manifest.py
index 35eb775..b0c3ac9 100644
--- a/tests/test_manifest.py
+++ b/tests/test_manifest.py
@@ -354,7 +354,7 @@ def test_notify_with_posargs():
     # delete my_session from the queue
     manifest.filter_by_name(())
 
-    assert session.posargs is cfg.posargs
+    assert session.posargs == cfg.posargs
     assert manifest.notify("my_session", posargs=["--an-arg"])
     assert session.posargs == ["--an-arg"]
 
diff --git a/tests/test_sessions.py b/tests/test_sessions.py
index 150c15e..518ab0c 100644
--- a/tests/test_sessions.py
+++ b/tests/test_sessions.py
@@ -66,9 +66,7 @@ class TestSession:
             signatures=["test"],
             func=func,
             global_config=_options.options.namespace(
-                posargs=mock.sentinel.posargs,
-                error_on_external_run=False,
-                install_only=False,
+                posargs=[], error_on_external_run=False, install_only=False
             ),
             manifest=mock.create_autospec(nox.manifest.Manifest),
         )
@@ -100,7 +98,7 @@ class TestSession:
 
         assert session.name is runner.friendly_name
         assert session.env is runner.venv.env
-        assert session.posargs is runner.global_config.posargs
+        assert session.posargs == runner.global_config.posargs
         assert session.virtualenv is runner.venv
         assert session.bin_paths is runner.venv.bin_paths
         assert session.bin is runner.venv.bin_paths[0]
@@ -351,7 +349,7 @@ class TestSession:
             name="test",
             signatures=["test"],
             func=mock.sentinel.func,
-            global_config=_options.options.namespace(posargs=mock.sentinel.posargs),
+            global_config=_options.options.namespace(posargs=[]),
             manifest=mock.create_autospec(nox.manifest.Manifest),
         )
         runner.venv = mock.create_autospec(nox.virtualenv.CondaEnv)
@@ -390,7 +388,7 @@ class TestSession:
             name="test",
             signatures=["test"],
             func=mock.sentinel.func,
-            global_config=_options.options.namespace(posargs=mock.sentinel.posargs),
+            global_config=_options.options.namespace(posargs=[]),
             manifest=mock.create_autospec(nox.manifest.Manifest),
         )
         runner.venv = mock.create_autospec(nox.virtualenv.CondaEnv)
@@ -447,7 +445,7 @@ class TestSession:
             name="test",
             signatures=["test"],
             func=mock.sentinel.func,
-            global_config=_options.options.namespace(posargs=mock.sentinel.posargs),
+            global_config=_options.options.namespace(posargs=[]),
             manifest=mock.create_autospec(nox.manifest.Manifest),
         )
         runner.venv = mock.create_autospec(nox.virtualenv.VirtualEnv)
@@ -476,7 +474,7 @@ class TestSession:
             name="test",
             signatures=["test"],
             func=mock.sentinel.func,
-            global_config=_options.options.namespace(posargs=mock.sentinel.posargs),
+            global_config=_options.options.namespace(posargs=[]),
             manifest=mock.create_autospec(nox.manifest.Manifest),
         )
         runner.venv = mock.create_autospec(nox.virtualenv.VirtualEnv)
@@ -564,7 +562,7 @@ class TestSessionRunner:
             global_config=_options.options.namespace(
                 noxfile=os.path.join(os.getcwd(), "noxfile.py"),
                 envdir="envdir",
-                posargs=mock.sentinel.posargs,
+                posargs=[],
                 reuse_existing_virtualenvs=False,
                 error_on_missing_interpreters=False,
             ),
@@ -580,7 +578,7 @@ class TestSessionRunner:
         assert runner.func is not None
         assert callable(runner.func)
         assert isinstance(runner.description, str)
-        assert runner.global_config.posargs == mock.sentinel.posargs
+        assert runner.global_config.posargs == []
         assert isinstance(runner.manifest, nox.manifest.Manifest)
 
     def test_str_and_friendly_name(self):
diff --git a/tests/test_tasks.py b/tests/test_tasks.py
index 65f3112..ca477ff 100644
--- a/tests/test_tasks.py
+++ b/tests/test_tasks.py
@@ -139,7 +139,7 @@ def test_discover_session_functions_decorator():
     mock_module = argparse.Namespace(
         __name__=foo.__module__, foo=foo, bar=bar, notasession=notasession
     )
-    config = _options.options.namespace(sessions=(), keywords=())
+    config = _options.options.namespace(sessions=(), keywords=(), posargs=())
 
     # Get the manifest and establish that it looks like what we expect.
     manifest = tasks.discover_manifest(mock_module, config)
@@ -149,7 +149,9 @@ def test_discover_session_functions_decorator():
 
 
 def test_filter_manifest():
-    config = _options.options.namespace(sessions=(), pythons=(), keywords=())
+    config = _options.options.namespace(
+        sessions=(), pythons=(), keywords=(), posargs=()
+    )
     manifest = Manifest({"foo": session_func, "bar": session_func}, config)
     return_value = tasks.filter_manifest(manifest, config)
     assert return_value is manifest
@@ -157,14 +159,18 @@ def test_filter_manifest():
 
 
 def test_filter_manifest_not_found():
-    config = _options.options.namespace(sessions=("baz",), pythons=(), keywords=())
+    config = _options.options.namespace(
+        sessions=("baz",), pythons=(), keywords=(), posargs=()
+    )
     manifest = Manifest({"foo": session_func, "bar": session_func}, config)
     return_value = tasks.filter_manifest(manifest, config)
     assert return_value == 3
 
 
 def test_filter_manifest_pythons():
-    config = _options.options.namespace(sessions=(), pythons=("3.8",), keywords=())
+    config = _options.options.namespace(
+        sessions=(), pythons=("3.8",), keywords=(), posargs=()
+    )
     manifest = Manifest(
         {"foo": session_func_with_python, "bar": session_func, "baz": session_func},
         config,
@@ -175,7 +181,9 @@ def test_filter_manifest_pythons():
 
 
 def test_filter_manifest_keywords():
-    config = _options.options.namespace(sessions=(), pythons=(), keywords="foo or bar")
+    config = _options.options.namespace(
+        sessions=(), pythons=(), keywords="foo or bar", posargs=()
+    )
     manifest = Manifest(
         {"foo": session_func, "bar": session_func, "baz": session_func}, config
     )
@@ -230,7 +238,7 @@ def test_verify_manifest_empty():
 
 
 def test_verify_manifest_nonempty():
-    config = _options.options.namespace(sessions=(), keywords=())
+    config = _options.options.namespace(sessions=(), keywords=(), posargs=())
     manifest = Manifest({"session": session_func}, config)
     return_value = tasks.verify_manifest_nonempty(manifest, global_config=config)
     assert return_value == manifest

I think we can also choose to be pragmatic here, and keep this as it is. After all, the mutability of session.posargs is undocumented and an implementation detail, and should not be used in Noxfiles. What do you think?

@theacodes What's your take on this?

Copy link
Contributor Author

@rmorshea rmorshea Mar 10, 2021

Choose a reason for hiding this comment

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

While we're thinking about making breaking change we should also consider turning posargs into a tuple. Having posargs be a list just seems like you're offering people enough rope to hang themselves with.

Copy link
Collaborator

@cjolowicz cjolowicz Mar 10, 2021

Choose a reason for hiding this comment

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

I agree that a tuple would be preferable. (Changing the type might not even constitute a breaking change; the exact type is not documented and most Noxfiles probably just unpack into session.run.) But I think we can merge this without addressing the issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create an issue to capture this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here: #400


@property
def description(self) -> Optional[str]:
Expand Down
22 changes: 18 additions & 4 deletions tests/test_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,11 @@ def create_mock_sessions():


def create_mock_config():
cfg = mock.sentinel.CONFIG
cfg = mock.sentinel.MOCKED_CONFIG
cfg.force_venv_backend = None
cfg.default_venv_backend = None
cfg.extra_pythons = None
cfg.posargs = []
return cfg


Expand Down Expand Up @@ -223,9 +224,7 @@ def session_func():
],
)
def test_extra_pythons(python, extra_pythons, expected):
cfg = mock.sentinel.CONFIG
cfg.force_venv_backend = None
cfg.default_venv_backend = None
cfg = create_mock_config()
cfg.extra_pythons = extra_pythons

manifest = Manifest({}, cfg)
Expand Down Expand Up @@ -345,6 +344,21 @@ def my_session(session):
assert len(manifest) == 1


def test_notify_with_posargs():
cfg = create_mock_config()
manifest = Manifest({}, cfg)

session = manifest.make_session("my_session", Func(lambda session: None))[0]
manifest.add_session(session)

# delete my_session from the queue
manifest.filter_by_name(())

assert session.posargs is cfg.posargs
assert manifest.notify("my_session", posargs=["--an-arg"])
assert session.posargs == ["--an-arg"]


def test_notify_error():
manifest = Manifest({}, create_mock_config())
with pytest.raises(ValueError):
Expand Down
6 changes: 5 additions & 1 deletion tests/test_sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,11 @@ def test_notify(self):

session.notify("other")

runner.manifest.notify.assert_called_once_with("other")
runner.manifest.notify.assert_called_once_with("other", None)

session.notify("other", posargs=["--an-arg"])

runner.manifest.notify.assert_called_with("other", ["--an-arg"])

def test_log(self, caplog):
caplog.set_level(logging.INFO)
Expand Down