Page MenuHomePhabricator

use instance_for_test in test_event_callback function
ClosedPublic

Authored by schrockn on Oct 29 2020, 10:08 PM.

Details

Summary

I was looking at this test investigating flakiness and I notice it was using the forbidden method for testing (DagsterInstance.local_temp()) I doubt this will fix the issue described in #3169 but a good excuse to replace this callsite.

Test Plan

BK

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

I think it might?

The thing that jumped out from that BK log output was

DagsterInstanceMigrationRequired: Instance is out of date and must be migrated (SqliteEventLogStorage for run 925f35e0-e1be-4d95-ba4e-d811849aed61). Database is at revision None, head is c34498c29964. Please run `dagster instance migrate`.

Dunno how that would happen unless the test pulled in an old instance somehow, so using instance_for_test() (which always creates a brand new instance) could very well help

This revision is now accepted and ready to land.Oct 29 2020, 10:42 PM

ok I no longer think that was it, I'm seeing this in other tests that use instance_for_test :) there's something funky going on with the event log.

got it. I reopen-ed https://github.com/dagster-io/dagster/issues/3169 for tracking if you want to put failures somewhere

Sent via Superhuman ( https://sprh.mn/?vip=schrockn@elementl.com )

We can find the real underlying issue using https://dagster.phacility.com/D4975, the migration error is masking the true cause