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

Bump tox from 4.11.4 to 4.12.0 #1606

Merged
merged 5 commits into from Jan 16, 2024
Merged

Bump tox from 4.11.4 to 4.12.0 #1606

merged 5 commits into from Jan 16, 2024

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Jan 14, 2024

Bumps tox from 4.11.4 to 4.12.0.

Release notes

Sourced from tox's releases.

4.12.0

What's Changed

New Contributors

Full Changelog: tox-dev/tox@4.11.4...4.12.0

Changelog

Sourced from tox's changelog.

v4.12.0 (2024-01-11)

Features - 4.12.0

- Always pass ``FORCE_COLOR`` and ``NO_COLOR`` to the environment (:issue:`3171`)

Bugfixes - 4.12.0

  • --parallel-no-spinner flag now implies --parallel (:issue:3158)

Improved Documentation - 4.12.0

- -Fix ``open an issue`` link in development.rst (:issue:`3179`)
Commits

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [tox](https://github.com/tox-dev/tox) from 4.11.4 to 4.12.0.
- [Release notes](https://github.com/tox-dev/tox/releases)
- [Changelog](https://github.com/tox-dev/tox/blob/main/docs/changelog.rst)
- [Commits](tox-dev/tox@4.11.4...4.12.0)

---
updated-dependencies:
- dependency-name: tox
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot bot added dependencies Pull requests that update a dependency file python Pull requests that update Python code labels Jan 14, 2024
freakboy3742
freakboy3742 previously approved these changes Jan 14, 2024
@rmartin16
Copy link
Member

fwiw: #1262 (comment)

may be easiest to continue masking FORCE_COLOR from pytest

@freakboy3742
Copy link
Member

fwiw: #1262 (comment)

may be easiest to continue masking FORCE_COLOR from pytest

Yeah... this is a hairy one.

Masking FORCE_COLOR is definitely the expedient option. However, it then means that we lose the effect of FORCE_COLOR in the tox environment - so we won't have color output for anything run in tox.

However, if you're running tests outside tox, and you have FORCE_COLOR set, the tests fail at present (e.g., FORCE_COLOR=True pytest will fail at present). So, we've got a slightly fragile test environment, which is something that should probably be fixed, regardless of how we fix this for the benefit of tox.

I think I've got a handle on how we can do this cleanly; I'll tag you on a review if I can pull something together.

@rmartin16
Copy link
Member

rmartin16 commented Jan 15, 2024

Good deal; I'll just throw this out there as well...if we get pytest to set TERM=dumb at startup, that should override anything in Rich from including ANSI sequences in the output.

@freakboy3742
Copy link
Member

@rmartin16 Ok - I've got an approach that works. Let me know what you think.

@rmartin16
Copy link
Member

This makes sense to me once I'm able to take it in; given it is just for tests, though, it feels perhaps a bit intrusive...

What are your thoughts on this?

It requires another dev dependency...but from some people were already pretty familiar with.

diff --git a/pyproject.toml b/pyproject.toml
index 01994b90..2e4b67e0 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -99,9 +99,10 @@ dev = [
     "pre-commit == 3.5.0 ; python_version < '3.9'",
     "pre-commit == 3.6.0 ; python_version >= '3.9'",
     "pytest == 7.4.4",
+    "pytest-env == 1.1.3",
     "pytest-xdist == 3.5.0",
     "setuptools_scm == 8.0.4",
-    "tox == 4.11.4",
+    "tox == 4.12.0",
 ]
 docs = [
     "furo == 2023.9.10",
@@ -225,6 +226,10 @@ multi_line_output = 3
 
 [tool.pytest.ini_options]
 testpaths = ["tests"]
+required_plugins = "pytest-env"
+env = [
+    # ensure Rich disables colored output
+    "TERM=dumb",
+]
 filterwarnings = [
     "error",
     # suppress until python-dateutil>2.8.2 is released (https://github.com/dateutil/dateutil/issues/1284)

@freakboy3742
Copy link
Member

This makes sense to me once I'm able to take it in; given it is just for tests, though, it feels perhaps a bit intrusive...

Yeah... I agree it's more complex that I'd like. However, that's mostly because Printer is a singleton with a runtime-environment-bound interpretation.

There might be a simpler approach that is based on Console instantiating an instance of Printer on first use - the PR will be larger (because it needs to work around all the existing singleton handling), but the final product should be easier to understand because it will be able to rely on simple attribute access and creation-time instantiation - which means the console itself could be configured to be "rich" for a single test.

What are your thoughts on this?

It requires another dev dependency...but from some people were already pretty familiar with.

Does this fix the problem though? Part of the reason that the solution is so baroque is that pytest imports the test module as part of discovery, which creates the Printer, which locks in the terminal interpretation. Does pytest-env modify the environment before test discovery imports occur? Or is it just a way to avoid the session-startup hook?

@rmartin16
Copy link
Member

What are your thoughts on this?
It requires another dev dependency...but from some people were already pretty familiar with.

Does this fix the problem though? Part of the reason that the solution is so baroque is that pytest imports the test module as part of discovery, which creates the Printer, which locks in the terminal interpretation. Does pytest-env modify the environment before test discovery imports occur? Or is it just a way to avoid the session-startup hook?

It fixes it for me on my Linux daily driver and my Windows 11 VM. I had quickly reviewed the pytest-env docs seeing if they explicitly mentioned this but didn't see anything. I'm definitely under the impression that pytest is loading the pytest-env hooks before actually importing the Briefcase tests.

Ultimately, though, I'm fine with either implementation.

@freakboy3742
Copy link
Member

@rmartin16 Ok - here's an alternate approach. I've made Printer not be a singleton. This means we no longer need the session cleanup handler, because the Console and Log objects are now created on every use; and they're also late instantiated, avoiding the environment problem.

My inclination is that this is an architectural change that is beneficial regardless; and it has the added bonus that it defers the instantiation of the log instance, so we can use environment variables to control the instantiation. We could potentially even add tests for how the log handles environment variable, because each test will create a fresh Log instance.

It also avoids a third-party plugin - while the pytest-env plugin doesn't look especially risky, my preference will always be to have less dependencies unless we're gaining significant functionality gains in exchange. Since we can get those gains by making an architectural change that is a good idea (IMHO) regardless, avoiding the plugin seems preferable.

Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

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

This is arguably the best way to mitigate the issue. Using pytest-env was mostly a recommendation for path of least resistance. Nitpick comments otherwise.

Comment on lines +18 to +20
printer = Printer()
console = Console(printer=printer)
logger = Log(printer=printer)
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Printer doesn't really need to be exposed up here in main; a console initialization function could wrap all this (and future logic) and return just the console and logger. (although, mostly indifferent if this happens now...or when future logic is added)

Copy link
Member

Choose a reason for hiding this comment

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

Possibly - but that likely requires a either more comprehensive refactor so that Log and Console are more closely bound (or even merged entirely); introducing another wrapper object, or a single purpose "construct both" method, that really only avoids creating the printer. At least for now, my inclination is to leave this as is, and be explicit about the fact that there's a single printer, which both the log and the console may write to.


@classmethod
def __call__(cls, *messages, stack_offset=5, show=True, **kwargs):
:param width: The width at which content should be wrapped.
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: the uninitiated may be misled by this naming and description of width. It sounds like it'll affect both what's printed to the console and the log....while it's only relevant to the log.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed- I've tweaked the naming.

@freakboy3742 freakboy3742 merged commit 1563842 into main Jan 16, 2024
45 checks passed
@dependabot dependabot bot deleted the dependabot/pip/tox-4.12.0 branch January 16, 2024 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants