Repository: theacodes/nox (python)

Issue:
Session.posargs should be immutable
There's no reason that `Session.posargs` should be mutated by users.

> ...a tuple would be preferable ...[and] might not even constitute a breaking change

_Originally posted by @cjolowicz in https://github.com/theacodes/nox/pull/397#discussion_r591857948_

Pull request:
Prevent sessions from modifying each other's posargs
Fixes #400 
Follow-up to #397 

This PR gives each session its own copy of `posargs`, thereby preventing mutations in one session to affect `posargs` in another session. 

```python
import nox

@nox.session
def test(session):
    session.posargs.extend(["-x"])
    session.install("pytest")
    session.run("pytest", *session.posargs)

@nox.session
def lint(session):
    args = [*session.posargs, "src"]
    session.install("flake8")
    session.run("flake8", *args)

# flake8: error: unrecognized arguments: -x
```

A test is added to check that `posargs` does not bleed through between sessions. Some existing tests are updated to fix breakage due to the change. There are three groups of tests that broke:

- 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

**Rationale:**

#397 introduced the ability to invoke `session.notify` with `posargs`, allowing one session to schedule another session with its own set of positional arguments. As a consequence, every session conceptually now has its own `posargs`; these arguments are no longer necessarily those passed on the command-line. Therefore it makes sense to prevent one session from accidentally mutating the `posargs` of other sessions.

`session.posargs` was already mutable before #397. But as all sessions received the same set of positional arguments, typical uses of positional arguments would restrict the run to a specific session. In such cases, mutating the shared copy of `posargs` would not lead to issues.

While it's conceivable that users would intentionally modify posargs in one session to affect another session, this technique would rely on an undocumented implementation detail. So I don't see a backwards compatibility issue here.

**Rejected Ideas:**

Using a `tuple` for `session.posargs`. This would break existing use patterns which rely on `session.posargs` supporting list operations, for example in the pip project. See https://github.com/theacodes/nox/issues/400#issuecomment-850632395 for more details.

Patch:
diff --git a/nox/sessions.py b/nox/sessions.py
index 31a28e7..cabaae1 100644
--- a/nox/sessions.py
+++ b/nox/sessions.py
@@ -506,7 +506,7 @@ class SessionRunner:
         self.global_config = global_config
         self.manifest = manifest
         self.venv: Optional[ProcessEnv] = None
-        self.posargs: List[str] = global_config.posargs
+        self.posargs: List[str] = global_config.posargs[:]
 
     @property
     def description(self) -> Optional[str]:


Test patch:
diff --git a/tests/test__option_set.py b/tests/test__option_set.py
index ae238f5..202473e 100644
--- a/tests/test__option_set.py
+++ b/tests/test__option_set.py
@@ -56,7 +56,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
         )
@@ -66,7 +66,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 e807619..4c2cf15 100644
--- a/tests/test_manifest.py
+++ b/tests/test_manifest.py
@@ -373,7 +373,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 3573b45..89df734 100644
--- a/tests/test_sessions.py
+++ b/tests/test_sessions.py
@@ -67,9 +67,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),
         )
@@ -101,7 +99,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]
@@ -371,7 +369,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)
@@ -435,7 +433,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)
@@ -492,7 +490,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)
@@ -521,7 +519,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)
@@ -556,6 +554,25 @@ class TestSession:
 
         runner.manifest.notify.assert_called_with("other", ["--an-arg"])
 
+    def test_posargs_are_not_shared_between_sessions(self, monkeypatch, tmp_path):
+        registry = {}
+        monkeypatch.setattr("nox.registry._REGISTRY", registry)
+
+        @nox.session(venv_backend="none")
+        def test(session):
+            session.posargs.extend(["-x"])
+
+        @nox.session(venv_backend="none")
+        def lint(session):
+            if "-x" in session.posargs:
+                raise RuntimeError("invalid option: -x")
+
+        config = _options.options.namespace(posargs=[])
+        manifest = nox.manifest.Manifest(registry, config)
+
+        assert manifest["test"].execute()
+        assert manifest["lint"].execute()
+
     def test_log(self, caplog):
         caplog.set_level(logging.INFO)
         session, _ = self.make_session_and_runner()
@@ -628,7 +645,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,
             ),
@@ -644,7 +661,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 967f605..9827cfc 100644
--- a/tests/test_tasks.py
+++ b/tests/test_tasks.py
@@ -140,7 +140,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)
@@ -150,7 +150,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
@@ -158,14 +160,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,
@@ -176,7 +182,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
     )
@@ -231,7 +239,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