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

ci: do not test a config that will never succeed #388

Merged
merged 1 commit into from
Mar 18, 2023

Conversation

ahayworth
Copy link
Contributor

In this test, we are asserting that the instrumentation is not
installed when the Rack constant is not present (see the before
block for this test case). We then go on to test that the configuration
is the "default" configuration that you'd get with a fresh installation.

The problem is that if the Rack constant is not present, then the
instrumentation-base code that sets all the defaults for our options
is never run (and logically, why would it?). So these test lines can
never actually succeed.

Unless, of course, the instrumentation object we're referring to is
not a pristine, new object. And in fact, depending on what order the
tests run in, our object is not a pristine, new object. If you run
basically any other test in this suite before, then we actually end up
recycling an object that is partially initialzied from a previous test,
and has an internal @config object that has some default options.

And so, the test is actually passing some times because of this
non-deterministic behavior. For example, if you run with SEED=1, then
the test suite passes (because other tests run first that initialize the
object completely). If you run with SEED=6372, it fails (because it is
the very first test run).

The real bug here is that we do not have proper test isolation. I
think that's actually a problem all over the code base, if I'm being
honest about it.

However, I elect not to fix that problem today. We'd need some way to
"reset" instrumentation and the registry in between tests (maybe not
that hard, honestly). That's more than I want to do on a Saturday
afternoon. So to fix the current issue, we just don't bother testing
things that logically could never pass anyways. What we actually care
about is that the instrumentation reports it was not installed, which
does work correctly as it is.

Fixes #387

In this test, we are asserting that the instrumentation is _not_
installed when the `Rack` constant is not present (see the `before`
block for this test case). We then go on to test that the configuration
is the "default" configuration that you'd get with a fresh installation.

The problem is that if the `Rack` constant is not present, then the
`instrumentation-base` code that sets all the defaults for our options
is never run (and logically, why would it?). So these test lines can
never actually succeed.

Unless, of course, the `instrumentation` object we're referring to is
_not_ a pristine, new object. And in fact, depending on what order the
tests run in, our object is _not_ a pristine, new object. If you run
basically any _other_ test in this suite before, then we actually end up
recycling an object that is partially initialzied from a previous test,
and has an internal `@config` object that has some default options.

And so, the test is actually passing some times because of this
non-deterministic behavior. For example, if you run with `SEED=1`, then
the test suite passes (because other tests run first that initialize the
object completely). If you run with `SEED=6372`, it fails (because it is
the very first test run).

The _real_ bug here is that we do not have proper test isolation. I
think that's actually a problem all over the code base, if I'm being
honest about it.

However, I elect not to fix that problem today. We'd need some way to
"reset" instrumentation and the registry in between tests (maybe not
_that_ hard, honestly). That's more than I want to do on a Saturday
afternoon. So to fix the current issue, we just don't bother testing
things that logically could never pass anyways. What we actually care
about is that the instrumentation reports it was not installed, which
does work correctly as it is.

Fixes #387
@ahayworth
Copy link
Contributor Author

Fixes #372

@ahayworth ahayworth linked an issue Mar 18, 2023 that may be closed by this pull request
@ahayworth ahayworth merged commit e5ba885 into main Mar 18, 2023
@ahayworth ahayworth deleted the ahayworth/rack-test-ordering-failure branch March 18, 2023 20:05
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.

Rack tests are non-deterministic Non Deterministic Test Rack
2 participants