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

Avoid a flaky warning caused by multi-threaded DB access #3006

Merged
merged 1 commit into from Jul 7, 2021

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jun 3, 2021

I've noticed that we occasionally see a warning about an uncaught exception thrown on one of the worker threads in test_drawing_from_recursive_strategy_is_thread_safe.

Unfortunately I don't have an example handy, but from memory at least one of the exceptions involved was due to threads not being able to access the database file. Since the database isn't relevant to this test, I've disabled it in the hopes that it will get rid of the warning.

@Zalathar Zalathar added the tests/build/CI about testing or deployment *of* Hypothesis label Jun 3, 2021
@Zalathar
Copy link
Contributor Author

Zalathar commented Jun 3, 2021

Ah, here's a different flaky warning in this test that I remember having seen earlier:

=============================== warnings summary ===============================
hypothesis-python/tests/nocover/test_recursive.py::test_drawing_from_recursive_strategy_is_thread_safe
  /Users/runner/work/hypothesis/hypothesis/hypothesis-python/.tox/py38-full/lib/python3.8/site-packages/_pytest/threadexception.py:75: PytestUnhandledThreadExceptionWarning: Exception in thread Thread-3
  
  Traceback (most recent call last):
    File "/Users/runner/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/threading.py", line 932, in _bootstrap_inner
      self.run()
    File "/Users/runner/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/threading.py", line 870, in run
      self._target(*self._args, **self._kwargs)
    File "/Users/runner/work/hypothesis/hypothesis/hypothesis-python/tests/nocover/test_recursive.py", line 141, in test
      @given(data=st.data())
    File "/Users/runner/work/hypothesis/hypothesis/hypothesis-python/.tox/py38-full/lib/python3.8/site-packages/hypothesis/core.py", line 1174, in wrapped_test
      raise the_error_hypothesis_found
    File "/Users/runner/work/hypothesis/hypothesis/hypothesis-python/.tox/py38-full/lib/python3.8/site-packages/hypothesis/core.py", line 1143, in wrapped_test
      state.run_engine()
    File "/Users/runner/work/hypothesis/hypothesis/hypothesis-python/.tox/py38-full/lib/python3.8/site-packages/hypothesis/core.py", line 806, in run_engine
      self.execute_once(
    File "/Users/runner/work/hypothesis/hypothesis/hypothesis-python/.tox/py38-full/lib/python3.8/site-packages/hypothesis/core.py", line 651, in execute_once
      self.__flaky(
    File "/Users/runner/work/hypothesis/hypothesis/hypothesis-python/.tox/py38-full/lib/python3.8/site-packages/hypothesis/core.py", line 879, in __flaky
      raise Flaky(message)
  hypothesis.errors.Flaky: Hypothesis test(data=data(...)) produces unreliable results: Falsified on the first call but did not on a subsequent one
  
    warnings.warn(pytest.PytestUnhandledThreadExceptionWarning(msg))

@Zalathar Zalathar marked this pull request as draft June 3, 2021 11:19
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Avoiding such a warning is clearly an improvement on the status quo, and we're careful to document that Hypothesis is not threadsafe anyway.

@Zac-HD
Copy link
Member

Zac-HD commented Jul 6, 2021

Hey @Zalathar - is this ready to merge? It looks like a marginal improvement on the status quo to me, and of course we can always open another PR with further work 🙂

@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 7, 2021

Yeah, feel free to merge this if you want. As you say it's a clear improvement even if it doesn't necessarily fix the original problem, and I don't have any specific plans to keep working on it in the foreseeable future.

@Zac-HD Zac-HD marked this pull request as ready for review July 7, 2021 12:08
@Zac-HD Zac-HD merged commit 720a86d into HypothesisWorks:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests/build/CI about testing or deployment *of* Hypothesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants