Page MenuHomePhabricator

Add test coverage of the pipeline to svg process using svg snapshots
ClosedPublic

Authored by bengotow on Thu, Aug 22, 8:43 PM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Summary

This diff adds high-level test coverage to the code in Dagit that converts pipelines to SVGs for display. The tests render the React components that generate the SVG and write actual SVG files to disk, allowing you to visually compare the expected and actual results as well as seeing the highlighted changed lines in the test output.

This diff also uses a different strategy than the rest of the tests to fetch / cache the "mock" data they require. Currently in the rest of the codebase you have to run dagit with a special env var, which prints a whole ton of GraphQL requests/responses on the page and then you copy-paste that into mockData.tsx. This is a little tough because that file is gigantic and re-generating it is a manual process. For these tests, the tests generate a refetch.sh shell script that is placed in the data folder. Running it curls the Dagit GraphQL endpoint and re-writes the JSON files on disk. I think we can switch the other tests to doing something like this soon for easier updating.

Test Plan

Run new tests!

Diff Detail

Repository
R1 dagster
Branch
bengotow/-dagre-fixes
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

bengotow created this revision.Thu, Aug 22, 8:43 PM
bengotow updated this revision to Diff 3951.Thu, Aug 22, 9:01 PM
  • Add a test for a composite, add package cache so the tests pass
bengotow updated this revision to Diff 3952.Thu, Aug 22, 9:02 PM
  • Add a test for a composite, add package cache so the tests pass
  1. We should definitely configure phab to hide those huge data files
  2. This is really cool! Can you post a screenshot of the visual diff? Really curious what that looks like.
Harbormaster completed remote builds in B3156: Diff 3952.

Yeah we definitely need to hide some of these files!

Right now if the tests fail, you can open up the files (eg: pipeline.svg and pipeline.actual.svg) and look at them manually to review the changes. If the changes are intentional you'd rename the .actual one and use it as the new baseline. When you commit the change, at least on GitHub, you can also view how the pipeline rendering was altered via this sexy viewer: https://github.blog/2014-10-06-svg-viewing-diffing/, which is kinda cool.

rename the snapshots dir to __snapshots__ and phab will hide it

or edit https://dagster.phacility.com/config/edit/differential.generated-paths/ but snapshots seems fine

alangenfeld requested changes to this revision.Mon, Aug 26, 10:32 PM
This revision now requires changes to proceed.Mon, Aug 26, 10:32 PM
bengotow updated this revision to Diff 4017.Tue, Aug 27, 4:01 PM
  • Move data / snapshots to dunder’d dirs
bengotow updated this revision to Diff 4018.Tue, Aug 27, 4:07 PM
  • Use path.join for windows-friendly paths

Oh nice! Didn't realize that was a convention - renamed this and we should be good to go.

alangenfeld added inline comments.Tue, Aug 27, 4:11 PM
js_modules/dagit/src/__tests__/Nylas Final Week Docs
1 ↗(On Diff #4018)

accidental file addition?

js_modules/dagit/src/graph/getFullSolidLayout.ts
181–202 ↗(On Diff #4018)

is this supposed to be here or in D844?

alangenfeld added inline comments.Tue, Aug 27, 4:13 PM
js_modules/dagit/src/__tests__/graph/SVGRendering.test.tsx
32–42

probably better if this just used the dagster-graphql cli instead of expecting a webserver to be up (unless i am overlooking something)

alangenfeld requested changes to this revision.Tue, Aug 27, 4:21 PM
This revision now requires changes to proceed.Tue, Aug 27, 4:21 PM

ahh good call! Will get this switched over to use the dagster-graphql CLI, didn't know about that 👍

js_modules/dagit/src/__tests__/Nylas Final Week Docs
1 ↗(On Diff #4018)

Whoops - wow, not sure how this got in here. RIP using VSCode for note taking... sorry!

bengotow updated this revision to Diff 4034.Tue, Aug 27, 6:05 PM
  • Remove accidental notes doc
bengotow updated this revision to Diff 4035.Tue, Aug 27, 6:08 PM

Fixing base

alangenfeld accepted this revision.Tue, Aug 27, 8:14 PM

please do the switch over to dagster-graphql before land - hopefully should be straight forward

This revision is now accepted and ready to land.Tue, Aug 27, 8:14 PM
bengotow closed this revision.Wed, Aug 28, 10:32 PM

Due to some arc land mishaps this merged in 1634aa48368675355515d04166ed26d0ead7e9f1