up

Woah this cleaned up nicely, great job! Glad to see the extra boolean flag and conditional mark go away.

Nice nice 👍

hmm, can you just move all of the other dagster tests under dagster_tests/general_tests? then you shouldn't need the ignores, right?

Nice work! It’s great to see the graphql tests finally run against Buildkite.

up

up

As discussed on Slack, might be worth to get rid of the scrub_key_from_dict helper and just do the dummy value replacement inline. Otherwise, looks great!

Awesome job consolidating these – I can tell it wasn't easy. Added an inline question about the use of the snapshot_friendly flag.

Yeah good point. In reality they are pretty connected though, so I'll form the connection in the next rev

feedback

fix lazy loading

I think this is a more than solid first pass, and can address schrock's comments in the next one. Would be great to get this landed for today's release.

feedback

Looks great! Solid, self contained example with the DBs 👍

Address feedback, fix lint errors, add tests

You may want to add a simple # TODO: remove dependency on legacy_examples inline right before the issue message.

Looks great. These tests are supposed to show users how to test their code, so it's worth cleaning them up. What do you think about just checking pipeline success status, rather than checking that each solid was successful?

Nice!

Niceee

up

My bad - must have been a bad amend

Looks good. Did you see any staleness issues?

This is great! Nice to see all those protected accesses go away.

up