Add cache integration support #16

goodboy · 2017-01-06T18:25:29Z

This adds support for stashing TestSets in the pytest cache between
runs. By default the lastfailed set is always inserted into the
shell namespace even when the user didn't pass --lf.

You can use:

cache <name> as <cache key> # create an entry
cache del <cache key> # delete and entry
cache # print a summary

Resolves #15

goodboy · 2017-01-06T18:26:35Z

Ping for review @vodik @moises-silva @dkerr

vodik

I think you might want to tighten your line parsing, but otherwise looks reasonable.

vodik · 2017-01-06T19:26:32Z

interactive/plugin.py

+
+    def get_cache_dict(self, path=None):
+        if path is None:
+            path = '/'.join(['pytest-interactive', 'cache'])


Where does this actually end up on the filesystem?

<pytest run dir>/.cache/v/pytest-interactive/cache

pytest stores lastfailed @ <pytest run dir>/.cache/v/lastfailed

Nice, glad they're following xdg

vodik · 2017-01-06T19:29:33Z

interactive/shell.py

+                # update the local shell's ns
+                if testset:
+                    self.shell.user_ns[cache_key] = testset
+                return


What happens if someone inputs 'cache foo as '?

vodik · 2017-01-06T19:30:17Z

interactive/shell.py

+                self.shell.user_ns.pop(cache_key)
+                self.tr.write(
+                    "Deleted cache entry for '{}'\n".format(cache_key))
+                return


Or something like 'cache del as ' actually?

hehe bad stuff.
So good point we need to validate names.
Any suggestion on how to do that?

Tokenize the string first, consume the tokens one by one.

tokens = line.split() # or use an alternative that respects quoting if you're feeling fancy assert tokens[0] == 'cache' if tokens[1] == 'del': assert len(tokens) == 3 # cache del foo name = tokens[2] # ... else: assert len(tokens) == 4 # cache foo as ... assert token[2] == 'as' name = tokens[1] # ...

Which will also more correctly handle the case of putting too many whitespaces. Since you don't strip after you partition, i wonder how the following inputs would be handled:

cache foo as bar <- two spaces between foo and as, github is just cleaning it up for me ☹️

cache some random string as there's more to come

cache as as as

Thanks for the feedback. Yes more stringent parsing is needed.
This seems like a good spot to bring in hypothesis when I finally get to #8...

I would also suggest requiring cache add and cache del, so you can correctly deal with a cache called del: cache del del... and cache add del...

@vodik so have add along side blah as foo?
I have to say I like the as/del as ptk gives you highlighting (yes i know it's hacky).

Take a step back and spec out how you want to parse the command:

operation = "add" | "del" identifier = [A-Za-z0-9] cmd = "cache" operation identifier ["as" identifier]

something like that, and unambigious. I'm not sure if you require "as", but if you don't, for example, what is the actual error here: 'cache del as'? Incorrect name? or missing identifier next to the as?

goodboy · 2017-01-06T19:34:36Z

Oh and obviously I should write some docs...

goodboy · 2017-01-09T17:13:05Z

@vodik fixed this up to simply use the add/del subcommand syntax.
I also added docs and think it's ready to land.

Let me know if there's anything I missed ;)

vodik

Better, but could still use some work.

vodik · 2017-01-09T17:21:36Z

interactive/shell.py

+                    "Deleted cache entry for '{}'\n".format(name))
+                return
+
+            elif first == 'add':


This doesn't match your documentation:

'0' selected >>> cache tt.test_setB.test_modes setb

hah! thanks.

vodik · 2017-01-09T17:22:40Z

interactive/shell.py

+                    self.shell.user_ns[target] = testset
+                return
+
+            self.tt.err("{} is invalid".format(line))


Probably not a bad idea to print either/both a usage line or a more specific error, if you can.

vodik · 2017-01-09T17:23:13Z

interactive/shell.py

+
+            if first == 'del':
+                # delete an entry
+                name = tokens[1]


So no error if length is greater than 3?

vodik

Okay, while I think you should spend a bit more effort on tightening up the error messages, this looks functional.

goodboy · 2017-01-09T17:49:32Z

@vodik what more would you like with error msgs?
Right now as far as my manual testing has gone all the exceptions are appropriate.

vodik · 2017-01-09T19:24:56Z

Looks good to me.

Actually, one last thing...

vodik

Yeah, looks fine to me now. Just two small comments, but I guess its a bit of scope creep - I could see the identifier validity problem as something more general

vodik · 2017-01-09T19:26:10Z

interactive/shell.py

+                    "Created cache entry for '{}'\n".format(name))
+                return
+
+            self.tt.err("{} is invalid. See %cache? for usage.".format(line))


Heh, for all the hard times I give you for putting unnecessary quotes around your format strings, here's actually a good place to put some in.

vodik · 2017-01-09T19:27:32Z

interactive/shell.py

+                # create a new entry
+                name, target = tokens[1:]
+                testset = self.ns_eval(name)
+                self.tt.set_cache_items(target, testset)


Oh, would you maybe want to validate that target is a valid python keyword? You know, starts with a letter and doesn't contain invalid characters like dashes?

Yep good call.

This adds support for stashing `TestSet`s in the pytest cache between runs. By default the `lastfailed` set is always inserted into the shell namespace even when the user didn't pass `--lf`. You can use: - cache # print a summary - cache add <name> <cache key> # create an entry - cache del <name> # delete and entry Resolves #15

goodboy · 2017-01-09T21:59:02Z

@vodik good enough?

goodboy self-assigned this Jan 6, 2017

vodik suggested changes Jan 6, 2017

View reviewed changes

goodboy force-pushed the cache_support branch from 3ef55fa to 11fbdf3 Compare January 9, 2017 17:08

vodik suggested changes Jan 9, 2017

View reviewed changes

goodboy force-pushed the cache_support branch 2 times, most recently from 8111986 to 659f6ab Compare January 9, 2017 17:42

vodik approved these changes Jan 9, 2017

View reviewed changes

Tyler Goodlet added 2 commits January 9, 2017 16:57

Add docs for %cache magic

8fb4e4c

goodboy force-pushed the cache_support branch from 659f6ab to 8fb4e4c Compare January 9, 2017 21:58

goodboy merged commit 83b3294 into master Jan 9, 2017

pyup-bot mentioned this pull request Nov 30, 2017

Update pytest-interactive to 0.1.4 bossjones/scarlett_os#182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache integration support #16

Add cache integration support #16

goodboy commented Jan 6, 2017 •

edited

Loading

goodboy commented Jan 6, 2017

vodik left a comment

vodik Jan 6, 2017

goodboy Jan 6, 2017

vodik Jan 6, 2017

vodik Jan 6, 2017

goodboy Jan 6, 2017

vodik Jan 6, 2017

goodboy Jan 6, 2017

vodik Jan 6, 2017 •

edited

Loading

goodboy Jan 6, 2017

vodik Jan 6, 2017

goodboy Jan 6, 2017 •

edited

Loading

vodik Jan 6, 2017

goodboy commented Jan 6, 2017

goodboy commented Jan 9, 2017

vodik left a comment

vodik Jan 9, 2017

goodboy Jan 9, 2017

vodik Jan 9, 2017

vodik Jan 9, 2017

vodik left a comment

goodboy commented Jan 9, 2017

vodik commented Jan 9, 2017

vodik left a comment

vodik Jan 9, 2017

goodboy Jan 9, 2017

vodik Jan 9, 2017

goodboy Jan 9, 2017

goodboy commented Jan 9, 2017

Add cache integration support #16

Add cache integration support #16

Conversation

goodboy commented Jan 6, 2017 • edited Loading

goodboy commented Jan 6, 2017

vodik left a 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

vodik Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodboy commented Jan 6, 2017

goodboy commented Jan 9, 2017

vodik left a 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

vodik left a comment

Choose a reason for hiding this comment

goodboy commented Jan 9, 2017

vodik commented Jan 9, 2017

vodik left a 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

goodboy commented Jan 9, 2017

goodboy commented Jan 6, 2017 •

edited

Loading

vodik Jan 6, 2017 •

edited

Loading

goodboy Jan 6, 2017 •

edited

Loading