Add pre and post commands by gaborbernat · Pull Request #1000 · tox-dev/tox

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



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


    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

    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

+        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

+
+        # 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

+            self.verbosity1("  {}$ {} >{}".format(popen.cwd, cmd_args_shell, popen.outpath))
        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

            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

-                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

-        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

 [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

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

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

base implementation

3ba22cc

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

Uh oh!

Conversation

gaborbernat commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborbernat Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asottile commented Sep 18, 2018

Uh oh!

codecov Bot commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gaborbernat commented Sep 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborbernat Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborbernat commented Sep 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaborbernat commented Sep 18, 2018 •

edited

Loading

gaborbernat Sep 18, 2018 •

edited

Loading

codecov Bot commented Sep 19, 2018 •

edited

Loading

gaborbernat Sep 19, 2018 •

edited

Loading