Repository: theacodes/nox (python)

Issue:
Change default behavior for `--error-on-missing-interpreters`
Is it possible to improve `--error-on-missing-interpreters` with "principle of least surprise" in mind? We don't want onerous failures during local dev, but the two changes are proposed:

- Mention the `--error-on-missing-interpreters` flag in the error / warning / success message if an interpreter was skipped
- Change `--error-on-missing-interpreters` to default **ON** (instead of **OFF**) if we've detected the code is in CI (`CI=true`) or in an automated environment (`sys.stdout.isatty()==False`)

----

**Original Content** (Original title: Test suite skipped with error "Python interpreter 3.8 not found.)

<!-- Thank you for filing a bug! Please feel free to answer as much or as little of this template as you can. -->

**Describe the bug**
<!-- Please be as detailed as possible! -->

The desired Python version was not available, the test suite is skipped entirely.

```
2020-08-21T17:46:46.1946078Z ##[group]Run nox
2020-08-21T17:46:46.1946291Z [36;1mnox[0m
2020-08-21T17:46:46.1990422Z shell: /bin/bash -e {0}
2020-08-21T17:46:46.1990565Z env:
2020-08-21T17:46:46.1990705Z   pythonLocation: /opt/hostedtoolcache/Python/3.7.8/x64
2020-08-21T17:46:46.1990822Z ##[endgroup]
2020-08-21T17:46:46.2954365Z Running session tests-3.8
2020-08-21T17:46:46.3002157Z Session tests-3.8 skipped: Python interpreter 3.8 not found.
```
![image](https://user-images.githubusercontent.com/24982/90919962-42013480-e39c-11ea-814f-68b20845a42d.png)



**How to reproduce**
<!-- If possible, include your Noxfile and how you invoked Nox -->

```
import nox

@nox.session(python=["3.7"])
def tests(session):
    """Run pytest suite"""
    session.create_tmp()
    session.install("-r", "requirements-test.txt")
    tests = session.posargs or ["tests/"]
    session.run("pytest", *tests)

# TODO: add lint, type checking, etc.
```

```
$ cat .github/workflows/data_prep_tools_test.yml 
# This workflow will install Python dependencies, run tests.
# For more information see: https://help.github.com/actions/language-and-framework-guides/using-python-with-github-actions

name: test

on:
  push:
    branches: [ master ]
  pull_request:
    branches: [ master ]
    
defaults:
  run:
    working-directory: tools

jobs:
  build:

    runs-on: ubuntu-latest
    strategy:
      matrix:
        python-version: [3.7]

    steps:
    - uses: actions/checkout@v2
    - name: Set up Python ${{ matrix.python-version }}
      uses: actions/setup-python@v2
      with:
        python-version: ${{ matrix.python-version }}
    - name: Install dependencies
      run: |
        pip install nox
    - name: Test with nox
      run: |
        nox
```

**Expected behavior**
<!-- What should have happened? -->

non-zero exit code

Pull request:
Missing interpreters now error the session on CI by default
Closes #340 

This PR adds a check to determine if nox is running on CI when evaluating sessions with missing interpreters so that such sessions fail by default when being run on CI providers.

This has the desired effect:

**Skipped locally showing CI warning**

<img width="911" alt="image" src="https://user-images.githubusercontent.com/62767721/152945814-5e56ff71-4a52-47f6-9387-866d659915db.png">

**CI=true causes an error**

<img width="518" alt="image" src="https://user-images.githubusercontent.com/62767721/152945916-9da4d627-183a-4d6d-a2c5-9d902aa6ab4d.png">

*Note non-zero exit code*

**`--error-on-missing-interpreters` for reference**

<img width="524" alt="image" src="https://user-images.githubusercontent.com/62767721/152945978-8a887048-7db0-443f-ba59-b97f846d4b65.png">

I have some questions I'd like people's input on:

- Should this be documented or just rely on the warning message?
- The warning message is kinda long, any other suggestions that get the point across? Maybe just say it will fail in CI and leave it there without mentioning `--error-on-missing-interpreters`? Given that it will show any time a session is skipped a long message could get annoying.
- Currently we just check for `CI` existing in `os.environ` should it explicitly check for true?

Patch:
diff --git a/docs/usage.rst b/docs/usage.rst
index 1309a84..e9b0c3a 100644
--- a/docs/usage.rst
+++ b/docs/usage.rst
@@ -218,7 +218,7 @@ If the Noxfile sets ``nox.options.stop_on_first_error``, you can override the No
 Failing sessions when the interpreter is missing
 ------------------------------------------------
 
-By default, Nox will skip sessions where the Python interpreter can't be found. If you want Nox to mark these sessions as failed, you can use ``--error-on-missing-interpreters``:
+By default, when not on CI, Nox will skip sessions where the Python interpreter can't be found. If you want Nox to mark these sessions as failed, you can use ``--error-on-missing-interpreters``:
 
 .. code-block:: console
 
@@ -226,6 +226,8 @@ By default, Nox will skip sessions where the Python interpreter can't be found.
 
 If the Noxfile sets ``nox.options.error_on_missing_interpreters``, you can override the Noxfile setting from the command line by using ``--no-error-on-missing-interpreters``.
 
+If being run on Continuous Integration (CI) systems, Nox will treat missing interpreters as errors by default to avoid sessions silently passing when the requested python interpreter is not installed. Nox does this by looking for an environment variable called ``CI`` which is a convention used by most CI providers.
+
 .. _opt-error-on-external-run:
 
 Disallowing external programs
diff --git a/nox/_option_set.py b/nox/_option_set.py
index 6c2ba62..1656860 100644
--- a/nox/_option_set.py
+++ b/nox/_option_set.py
@@ -157,6 +157,7 @@ def make_flag_pair(
     name: str,
     enable_flags: tuple[str, str] | tuple[str],
     disable_flags: tuple[str],
+    default: bool = False,
     **kwargs: Any,
 ) -> tuple[Option, Option]:
     """Returns two options - one to enable a behavior and another to disable it.
@@ -172,6 +173,7 @@ def make_flag_pair(
         *enable_flags,
         noxfile=True,
         merge_func=functools.partial(flag_pair_merge_func, name, disable_name),
+        default=default,
         **kwargs,
     )
 
diff --git a/nox/_options.py b/nox/_options.py
index bdb032b..b61c41c 100644
--- a/nox/_options.py
+++ b/nox/_options.py
@@ -419,6 +419,7 @@ options.add_options(
         ("--no-error-on-missing-interpreters",),
         group=options.groups["execution"],
         help="Error instead of skipping sessions if an interpreter can not be located.",
+        default="CI" in os.environ,
     ),
     *_option_set.make_flag_pair(
         "error_on_external_run",
diff --git a/nox/sessions.py b/nox/sessions.py
index b3d9c8a..75fb671 100644
--- a/nox/sessions.py
+++ b/nox/sessions.py
@@ -694,6 +694,9 @@ class SessionRunner:
             if self.global_config.error_on_missing_interpreters:
                 return Result(self, Status.FAILED, reason=str(exc))
             else:
+                logger.warning(
+                    "Missing interpreters will error by default on CI systems."
+                )
                 return Result(self, Status.SKIPPED, reason=str(exc))
 
         except _SessionQuit as exc:


Test patch:
diff --git a/tests/test_sessions.py b/tests/test_sessions.py
index e753850..a71d1f9 100644
--- a/tests/test_sessions.py
+++ b/tests/test_sessions.py
@@ -749,7 +749,7 @@ class TestSessionRunner:
                 envdir="envdir",
                 posargs=[],
                 reuse_existing_virtualenvs=False,
-                error_on_missing_interpreters=False,
+                error_on_missing_interpreters="CI" in os.environ,
             ),
             manifest=mock.create_autospec(nox.manifest.Manifest),
         )
@@ -904,7 +904,11 @@ class TestSessionRunner:
         assert result.status == nox.sessions.Status.SKIPPED
         assert "no parameters" in result.reason
 
-    def test_execute_skip_missing_interpreter(self):
+    def test_execute_skip_missing_interpreter(self, caplog, monkeypatch):
+        # Important to have this first here as the runner will pick it up
+        # to set default for --error-on-missing-interpreters
+        monkeypatch.delenv("CI", raising=False)
+
         runner = self.make_runner_with_mock_venv()
         runner._create_venv.side_effect = nox.virtualenv.InterpreterNotFound("meep")
 
@@ -912,6 +916,19 @@ class TestSessionRunner:
 
         assert result.status == nox.sessions.Status.SKIPPED
         assert "meep" in result.reason
+        assert (
+            "Missing interpreters will error by default on CI systems." in caplog.text
+        )
+
+    def test_execute_missing_interpreter_on_CI(self, monkeypatch):
+        monkeypatch.setenv("CI", "True")
+        runner = self.make_runner_with_mock_venv()
+        runner._create_venv.side_effect = nox.virtualenv.InterpreterNotFound("meep")
+
+        result = runner.execute()
+
+        assert result.status == nox.sessions.Status.FAILED
+        assert "meep" in result.reason
 
     def test_execute_error_missing_interpreter(self):
         runner = self.make_runner_with_mock_venv()