Add pre and post commands #1000

gaborbernat · 2018-09-18T20:57:38Z

Resolves #167.
Resolves #1003.
Resolves #1004.
Fixup for #987 and #851.

PS. Once we resolve and merge this we'll release 3.4.0.

asottile · 2018-09-18T21:01:13Z

src/tox/venv.py

@@ -532,6 +542,31 @@ def tox_runtest(venv, redirect):
    return True  # Return non-None to indicate plugin has completed


+@tox.hookimpl
+def tox_runtest_pre(venv):


I think these should be separate hooks for maximum compatibility with previous plugins implementing these (otherwise this is an added responsibility that every existing plugin that implements tox_runtest_pre now needs to also do!)

The only added responsibility we're implying here is that the venv.status should be still zero after tox_runtest_pre. I don't think anyone modified that until now 🤔 So what other responsibilities am I missing? (would prefer not to add new hooks, to pollute our hook namespace even more).

ah ok, these aren't firstresult=True (they maybe should be? hard to say) nevermind this then

only the commands are first result 👍 the setup and teardown (pre and post) are not. And makes sense as while we don't want users to execute the command evaluation multiple times; we do want to allow them to setup/teardown their plugin.

asottile · 2018-09-18T22:35:54Z

jfc codecov SETTLE DOWN 😆

codecov · 2018-09-19T06:20:00Z

Codecov Report

Merging #1000 into master will decrease coverage by 0.21%.
The diff coverage is 81.48%.

@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
- Coverage   69.78%   69.57%   -0.22%     
==========================================
  Files          14       14              
  Lines        3406     3435      +29     
  Branches      453      458       +5     
==========================================
+ Hits         2377     2390      +13     
- Misses        965      980      +15     
- Partials       64       65       +1

Impacted Files	Coverage Δ
src/tox/package.py	`57.67% <0%> (-1.25%)`	⬇️
src/tox/config.py	`65.1% <100%> (-0.47%)`	⬇️
src/tox/session.py	`77.72% <50%> (ø)`	⬆️
src/tox/venv.py	`76.26% <90.47%> (+0.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70bfb83...f70c541. Read the comment docs.

gaborbernat · 2018-09-19T20:53:44Z

@asottile While trying to write tests for this and making them pass discovered two feature request that helped me understand what's happening

reasoning for venv recreation,
only update-ing .tox.config1 when it changes, not always;

And two user affecting bugs:

PEP-517 packaging
and command quoting being incorrect.

And two internal bugs (thing happened to work by chance, but not by design):

python md5 being zero when there were dependencies because of variable re-usage;
and pragma no cover being ignored.

As such this PR became a sort of longer one. However due to the iterative nature of these would be troublesome to split it up. As such I would appreciate to review and merge it as one.

asottile · 2018-09-19T21:28:49Z

src/tox/_pytestplugin.py

@@ -69,7 +69,7 @@ def cmd(request, capfd, monkeypatch):
    request.addfinalizer(py.path.local().chdir)

    def run(*argv):
-        key = str(b"PYTHONPATH")
+        key = b"PYTHONPATH" if six.PY2 else "PYTHONPATH"


"native" string can just be str("PYTHONPATH") -- though as written is just as explicit and correct

asottile · 2018-09-19T21:29:36Z

src/tox/_pytestplugin.py

@@ -69,7 +69,7 @@ def cmd(request, capfd, monkeypatch):
    request.addfinalizer(py.path.local().chdir)

    def run(*argv):
-        key = str(b"PYTHONPATH")
+        key = b"PYTHONPATH" if six.PY2 else "PYTHONPATH"
        python_paths = (i for i in (str(os.getcwd()), os.getenv(key)) if i)


hmm, I can't think of a situation where getcwd() returns non-native string -- I suspect the str(...) call here is superfluous

asottile · 2018-09-19T21:31:58Z

src/tox/_pytestplugin.py

+        try:
+            ret = res._popen(cmd, **kwargs)
+        except tox.exception.InvocationError as exception:  # pragma: no cover
+            ret = exception  # pragma: no cover


p3: this no cover comment shouldn't be necessary since the except block itself is no-covered

asottile · 2018-09-19T21:33:05Z

src/tox/package.py

+
+        # need to start with an empty (but existing) source distribution folder
+        if config.distdir.exists():
+            config.distdir.remove(rec=1, ignore_errors=True)


potential race here (exists check finishes, other process creates), though I suspect it doesn't matter -- though could this just be config.distdir.remove(rec=1, ignore_errors=True)?

that was my first solution, until the test started failing; seems rec=1 is only active if the folder exists because of https://github.com/pytest-dev/py/blob/master/py/_path/local.py#L204. Long story short you can only delete if already exists...

🤦‍♂️ :man_gesturing_no: 😭

asottile · 2018-09-19T21:34:19Z

src/tox/session.py

        else:
-            self.verbosity1("  {}$ {} ".format(popen.cwd, cmd))
+            self.verbosity1("  {}$ {} ".format(popen.cwd, cmd_args_shell))


oh nice, this is the shlex fix in the output?

yeah should be 👍

asottile · 2018-09-19T21:35:18Z

src/tox/venv.py

@@ -35,13 +35,13 @@ def writeconfig(self, path):
            lines.append("{} {}".format(*dep))
        path.ensure()
        path.write("\n".join(lines))
+        return lines


(perhaps I'm not seeing it) is this return value used?

indeed not needed 👍

asottile · 2018-09-19T21:36:28Z

src/tox/venv.py

-                else self.deps == other.deps
-            )
-        )
+    def matches(self, other, deps_matches_subset=False, provide_reason=False):


this function (if annotated) would be Union[Tuple[bool, str], bool] -- does it make sense to instead have two functions: one without the reason and one with the reason?

I would like to avoid duplication of the logic though.

ah yeah you can just do:

def whatever_with_reason(...): return True, 'reason' def whatever_without_reason(...): ret, _ = whatever_with_reason(**kwargs) return ret

or something like that 🤷‍♂️

I see, my idea was that only makes the code more type correct; which is not really needed until we implement type annotations (probably once we drop Python 2 and 3.4).

ah yeah I guess I didn't put my justifcation, my bad!

It would also be nice so we don't have a boolean trap parameter -- especially until there's enforced keyword-only-arguments in python3 they're a pretty easy pit to fall into in python where you accidentally pass too many positional arguments and it splats over a defaulted keyword argument

asottile · 2018-09-19T21:37:24Z

src/tox/venv.py

-        name = output.strip()
-        args = [self.envconfig.envpython, "-c", "import sys; print(sys.path)"]
+        name = next(
+            (i for i in output.split("\n") if i and not i.startswith("pydev debugger:")), ""


is this left over from a debugging session?

More like a small fixup 👍 Ideally people should be able to run the test suite in debug mode too with pydev (both eclipse and Pycharm). For that to work this is needed.

asottile · 2018-09-19T21:38:18Z

tox.ini

@@ -107,7 +107,6 @@ branch = true
 [coverage:report]
 skip_covered = True
 show_missing = True
-exclude_lines = if __name__ == ["']__main__["']:


ah yeah here's what I usually put here:

https://github.com/asottile/all-repos/blob/756c89aa5513235a6f152ddefad311a38470974f/.coveragerc#L15-L28

gaborbernat · 2018-09-19T22:38:09Z

Made PR feedback; not all should be ok.

support vscode

ccb612c

gaborbernat requested a review from a team September 18, 2018 20:57

asottile reviewed Sep 18, 2018

View reviewed changes

gaborbernat force-pushed the 167 branch 3 times, most recently from 78dff35 to d1d646f Compare September 18, 2018 21:35

tox-dev deleted a comment from codecov bot Sep 19, 2018

gaborbernat force-pushed the 167 branch from d1d646f to 1f49909 Compare September 19, 2018 06:19

gaborbernat force-pushed the 167 branch 5 times, most recently from cd53fb7 to 08d9d5c Compare September 19, 2018 20:35

gaborbernat changed the title ~~[WIP] Add pre and post commands~~ Add pre and post commands Sep 19, 2018

gaborbernat force-pushed the 167 branch from 08d9d5c to 0726b72 Compare September 19, 2018 20:40

base implementation

3ba22cc

gaborbernat force-pushed the 167 branch from 0726b72 to 3ba22cc Compare September 19, 2018 20:44

PR feedback

478ea73

asottile reviewed Sep 19, 2018

View reviewed changes

asottile approved these changes Sep 19, 2018

View reviewed changes

gaborbernat merged commit 4269996 into tox-dev:master Sep 20, 2018

gaborbernat deleted the 167 branch September 20, 2018 06:34

AndreMiras mentioned this pull request Sep 20, 2018

Tox pre-install merged upstream AndreMiras/EtherollApp#113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre and post commands #1000

Add pre and post commands #1000

gaborbernat commented Sep 18, 2018 •

edited

asottile Sep 18, 2018

gaborbernat Sep 18, 2018

asottile Sep 18, 2018

gaborbernat Sep 18, 2018 •

edited

asottile commented Sep 18, 2018

codecov bot commented Sep 19, 2018 •

edited

gaborbernat commented Sep 19, 2018

asottile Sep 19, 2018

asottile Sep 19, 2018

asottile Sep 19, 2018

asottile Sep 19, 2018

gaborbernat Sep 19, 2018 •

edited

asottile Sep 19, 2018

asottile Sep 19, 2018

gaborbernat Sep 19, 2018

asottile Sep 19, 2018

gaborbernat Sep 19, 2018

asottile Sep 19, 2018

gaborbernat Sep 19, 2018

asottile Sep 19, 2018

gaborbernat Sep 19, 2018

asottile Sep 19, 2018

asottile Sep 19, 2018

gaborbernat Sep 19, 2018

asottile Sep 19, 2018

gaborbernat commented Sep 19, 2018

Add pre and post commands #1000

Add pre and post commands #1000

Conversation

gaborbernat commented Sep 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat Sep 18, 2018 • edited

Choose a reason for hiding this comment

asottile commented Sep 18, 2018

codecov bot commented Sep 19, 2018 • edited

Codecov Report

gaborbernat commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat Sep 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat commented Sep 19, 2018

gaborbernat commented Sep 18, 2018 •

edited

gaborbernat Sep 18, 2018 •

edited

codecov bot commented Sep 19, 2018 •

edited

gaborbernat Sep 19, 2018 •

edited