set local session posargs via notify #397

rmorshea · 2021-03-07T10:05:05Z

Closes: #391

Once the basic API has been reviewed and accepted I'll add tests for this feature.

cjolowicz

Looks great, I like the approach with the property setter :) Public API stays read-only, which is good. I left some minor suggestions inline, what do you think? Otherwise, I think this just needs tests and an update to the docstring for session.notify.

nox/manifest.py

nox/sessions.py

Also SessionRunner.notify did not need to be a property.

cjolowicz

Thanks @rmorshea this is pretty much good to go :) I just noticed a few more small things, could you take another look? Comments are inline.

nox/manifest.py

nox/sessions.py

cjolowicz · 2021-03-09T09:12:19Z

nox/sessions.py

@@ -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


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.

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?

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.

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.

I'll create an issue to capture this.

See here: #400

cjolowicz

LGTM

I'd be happy for this to be merged without making session.posargs immutable. (To be clear it's already mutable on the main branch, this PR does not change that.) We can deal with that in a separate PR if required.

rmorshea · 2021-03-10T21:21:45Z

@cjolowicz I think this is good to merge.

rmorshea · 2021-03-11T20:42:27Z

ping: @cjolowicz - I think this is good to merge

theacodes · 2021-03-12T03:04:07Z

Looks great, thanks, y'all!

rmorshea force-pushed the session-posargs branch from c0cf017 to 36a3471 Compare March 7, 2021 10:06

cjolowicz reviewed Mar 7, 2021

View reviewed changes

nox/manifest.py Show resolved Hide resolved

nox/manifest.py Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

nox/sessions.py Outdated Show resolved Hide resolved

rmorshea mentioned this pull request Mar 9, 2021

Better explain Session.notify #398

Closed

rmorshea changed the title ~~add local session posargs via notify~~ set local session posargs via notify Mar 9, 2021

rmorshea force-pushed the session-posargs branch from f48be2b to 91fc9cb Compare March 9, 2021 03:55

rmorshea added 3 commits March 8, 2021 19:57

add local session posargs via notify

b79cdd8

add docs for notify(posargs=...)

ed41e13

Also SessionRunner.notify did not need to be a property.

fix tests after notify(posargs=...) addition

dc115dd

rmorshea force-pushed the session-posargs branch from 91fc9cb to c7f3954 Compare March 9, 2021 03:57

add test for notified posargs

06a01ec

rmorshea force-pushed the session-posargs branch from c7f3954 to 06a01ec Compare March 9, 2021 06:41

rmorshea requested a review from cjolowicz March 9, 2021 07:50

cjolowicz reviewed Mar 9, 2021

View reviewed changes

rmorshea added 2 commits March 10, 2021 11:16

Add posargs to docstring for Manifest.notify

db4ed2f

Fix typo in docstring

4a66fe0

cjolowicz approved these changes Mar 10, 2021

View reviewed changes

rmorshea mentioned this pull request Mar 10, 2021

Session.posargs should be immutable #400

Closed

Merge branch 'main' into session-posargs

a54a859

theacodes merged commit 3240441 into wntrblm:main Mar 12, 2021

This was referenced Jun 5, 2021

Prevent sessions from modifying each other's posargs #439

Merged

Release 2021.6.6 #440

Merged

mlech-reef mentioned this pull request Oct 16, 2021

conda python310 maciej-lech/nox#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set local session posargs via notify #397

set local session posargs via notify #397

rmorshea commented Mar 7, 2021

cjolowicz left a comment

cjolowicz left a comment

cjolowicz Mar 9, 2021

cjolowicz Mar 9, 2021

rmorshea Mar 10, 2021 •

edited

cjolowicz Mar 10, 2021 •

edited

rmorshea Mar 10, 2021

rmorshea Mar 10, 2021

cjolowicz left a comment •

edited

rmorshea commented Mar 10, 2021

rmorshea commented Mar 11, 2021

theacodes commented Mar 12, 2021

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

set local session posargs via notify #397

set local session posargs via notify #397

Conversation

rmorshea commented Mar 7, 2021

cjolowicz left a comment

Choose a reason for hiding this comment

cjolowicz left a comment

Choose a reason for hiding this comment

cjolowicz Mar 9, 2021

Choose a reason for hiding this comment

cjolowicz Mar 9, 2021

Choose a reason for hiding this comment

rmorshea Mar 10, 2021 • edited

Choose a reason for hiding this comment

cjolowicz Mar 10, 2021 • edited

Choose a reason for hiding this comment

rmorshea Mar 10, 2021

Choose a reason for hiding this comment

rmorshea Mar 10, 2021

Choose a reason for hiding this comment

cjolowicz left a comment • edited

Choose a reason for hiding this comment

rmorshea commented Mar 10, 2021

rmorshea commented Mar 11, 2021

theacodes commented Mar 12, 2021

rmorshea Mar 10, 2021 •

edited

cjolowicz Mar 10, 2021 •

edited

cjolowicz left a comment •

edited