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

escalation.belongs_to cache is broken #1041

Closed
bukzor opened this issue Dec 19, 2017 · 2 comments · Fixed by #1044
Closed

escalation.belongs_to cache is broken #1041

bukzor opened this issue Dec 19, 2017 · 2 comments · Fixed by #1044

Comments

@bukzor
Copy link
Contributor

bukzor commented Dec 19, 2017

The problem is that the cache is checked with the incoming string type, but the string type changes before the value is inserted:

A simple (but not elegant) fix:

--- hypothesis/internal/escalation.py
+++ hypothesis/internal/escalation.py
@@ -34,13 +34,14 @@ def belongs_to(package):
     cache = {text_type: {}, binary_type: {}}
 
     def accept(filepath):
+        ftype = type(filepath)
         try:
-            return cache[type(filepath)][filepath]
+            return cache[ftype][filepath]
         except KeyError:
             pass
         filepath = encoded_filepath(filepath)
         result = os.path.abspath(filepath).startswith(root)
-        cache[type(filepath)][filepath] = result
+        cache[ftype][filepath] = result
         return result
     accept.__name__ = 'is_%s_file' % (package.__name__,)
     return accept
@Zac-HD
Copy link
Member

Zac-HD commented Dec 20, 2017

This looks a like a good fix to me - follow the contributing guide and make a pull!

@bukzor
Copy link
Contributor Author

bukzor commented Dec 20, 2017

Do you want to see a regression test for this? If so, how would it work?

bukzor added a commit to bukzor/hypothesis that referenced this issue Dec 20, 2017
bukzor added a commit to bukzor/hypothesis that referenced this issue Dec 20, 2017
bukzor added a commit to bukzor/hypothesis that referenced this issue Dec 22, 2017
bukzor added a commit to bukzor/hypothesis that referenced this issue Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants