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

Resolve the iterator outside of the hub context to not mess with the state of the RNG #23

Closed
wants to merge 1 commit into from

Conversation

abo-abo
Copy link

@abo-abo abo-abo commented Aug 22, 2023

Hi. We're making use of this pytest fixture:

@pytest.fixture(autouse=True)
def determinism(request: Any) -> None:
    """Re-seed the RNG differently for every test."""
    random.seed(request.module.__name__ + request.node.name)

We also make use of https://pypi.org/project/pytest-snapshot/. And when pytest-sentry is installed, it breaks our tests because somehow the RNG state gets modified.

This PR fixes our problem and I'm wondering if it's also an acceptable default for everyone.

If not, could we have a way to configure pytest-sentry so that we don't have to use a fork?

So that it does not mess with the state of the RNG.
@untitaker
Copy link
Member

untitaker commented Aug 22, 2023

I suspect what happens is that _start_transaction internally runs these lines: https://github.com/getsentry/sentry-python/blob/f4c19e168d15fbb7caa942333d048a85f147045c/sentry_sdk/tracing.py#L133-L134

and so uuid4().hex advances the state of your RNG

with your patch, since the correct hub is not active, _start_transaction will see that the SDK is not configured and short-circuit to do nothing. this breaks performance monitoring.

can you run more experiments where you disable _start_transaction or hardcode the return value of uuid4 in the SDK?

Also I assume disabling performance monitoring (as per the README) will have the same effect. Ah maybe not. Not sure.

@untitaker
Copy link
Member

Hi. We're making use of this pytest fixture:

Also I'm puzzled by this because this fixture does not seem like a good idea to me? If you have indeterministic code that behaves incorrectly sometimes depending on RNG, your testsuite will not catch it because each test is seeded with the same value every time.

For example, this test case always passes:

import random
import pytest

@pytest.fixture(autouse=True)
def determinism(request) -> None:
    """Re-seed the RNG differently for every test."""
    random.seed(request.module.__name__ + request.node.name)

def test_foo():
    assert random.random() == 0.725234044008829

but only on my machine, and only if the file is named lol.py.

@untitaker
Copy link
Member

closing because of no reply

@untitaker untitaker closed this Nov 22, 2024
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 this pull request may close these issues.

2 participants