Page MenuHomeElementl

Fix schreenshots test: Part 1
ClosedPublic

Authored by yichendai on Nov 20 2020, 9:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 12, 12:05 AM
Unknown Object (File)
Apr 9 2023, 3:46 PM
Unknown Object (File)
Apr 2 2023, 7:54 AM
Unknown Object (File)
Mar 19 2023, 5:14 AM
Unknown Object (File)
Mar 16 2023, 11:54 PM
Unknown Object (File)
Jan 28 2023, 5:19 PM
Unknown Object (File)
Dec 26 2022, 6:37 PM
Unknown Object (File)
Nov 28 2022, 7:27 AM
Subscribers

Details

Summary

Update all the screenshots for pictures in the Tutorials of Docs; not including images in Overviews.

Plan to add tests for images in Overview in another diff.

Test Plan

Need more eyes to double check since there are many image comparisons.

  • Run docs.dagster.io locally to see if images match texts.
  • Compare images diff under under docs/next/public/assets/images/tutorial

Diff Detail

Repository
R1 dagster
Branch
docspic
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yichendai edited the test plan for this revision. (Show Details)
yichendai edited the summary of this revision. (Show Details)

Update image for playground view in execute first pipeline

Change all the executeButton definition

Update: Allow a hover error to be seen and fix all the new URLs

Update: - Fix the rest of codemirror textarea errors

max requested changes to this revision.Dec 1 2020, 12:54 AM
max added inline comments.
docs/__image_snapshots__/composite_solids_expanded-snap.png
1

let's zoom this one in

docs/__image_snapshots__/config_figure_one-snap.png
1

why are there config errors?

docs/__image_snapshots__/config_figure_two-snap.png
1

i don't think this is illustrating the config anymore?

docs/__image_snapshots__/custom_types_bad_data-snap.png
1

what is this meant to illustrate?

docs/__image_snapshots__/reexecution_errors-snap.png
1

i think we don't want this filesystem tooltip and there's an extraneous error on the key

docs/generate_screenshots.test.js
125

remove this

145–146

i don't think this will work -- you are using an autogenerated class here that won't be the same on following runs. you need to figure out how to identify the element to hover over without this

145–146

same as above

145–146

can you wait for something specific instead?

145–146

can you wait for something specific instead?

145–146

can you wait for something specific instead?

182

can you get rid of this sleep and wait for something specific? or change the waitUntil param on the previous call?

183

maybe better to have a long timeout on this (10 seconds or sth) rather than hang forever if it fails

204

an you wait for something specific on the new tab instead?

docs/next/public/assets/images/tutorial/complex_pipeline_figure_one.png
1

i think we should probably ignore this directory for diffs --

This revision now requires changes to proceed.Dec 1 2020, 12:54 AM
docs/__image_snapshots__/config_figure_one-snap.png
1

solid sort_by_calories should be inserted in front of config. But this red highlighting still needs to report as a bug.

docs/__image_snapshots__/config_figure_two-snap.png
1

I see. It should be modified to click on sort_by_calories.

docs/__image_snapshots__/custom_types_bad_data-snap.png
1

In the Expections, it should help check if the result is the expected one.

yichendai added inline comments.
docs/generate_screenshots.test.js
145–146

This class RunPreview__Item-sc-5v37v5-4 PySGd stays the same for different runs. However, when the frontend make some changes, it has to be updated.

docs/next/public/assets/images/tutorial/complex_pipeline_figure_one.png
1

I'll rename_image_snapshots_ to _generated_ to reduce double work for review in next update.

yichendai marked 4 inline comments as done and an inline comment as not done.

Update: Move image_snapshots under generated to hide images generated by this test on diff and complete all the inline comments except for scrolling left for reexecution_error.png and optimizing some codes in the script.

docs/__image_snapshots__/config_figure_two-snap.png
1

Double-checked with docs: The correct version of this image should replace config_figure_one.png, illustrating the human readable description of the config schema of reverse. The original config_figure_one.png of run_config in playground view is not used in docs.

Fixed in next update.

Update: Use custom path dir to store snapshots (previous update was done manually)

docs/next/public/assets/images/tutorial/complex_pipeline_figure_one.png
1

I'll rename_image_snapshots_ to _generated_ to reduce double work for review in next update.

We should only review images under docs/next/public/assets/images/tutorial.

Snapshots are hidden by set the config of Phabricator .

Update: Clean sleep for n secs. Some unnecessary waitings are removed and some are changed to wait for specific selector.

yichendai marked an inline comment as done.

Update: Use timeout on next selector to prevent hanging forever resulting from previous failure

Till now, all the fixes have been done according to the inline comments in old Diff #26596

yichendai added inline comments.
js_modules/dagit/src/gaant/GaantChart.tsx
502 ↗(On Diff #26963)

Add a title on react component to help query select a specific executed solid. @dish

yichendai edited the test plan for this revision. (Show Details)
yichendai added a reviewer: sashank.
yichendai removed a reviewer: dish.
yichendai edited the test plan for this revision. (Show Details)

Update: Fix inputs_figure_seven.png having double config error messages after going through all the images in the tutorials

Happy weekend!

Macro chefkiss:

This looks good to me! Thanks for doing this.

Two comments before accepting this:

  1. Do we really need to commit the pngs in docs/__generated__ / into the codebase? they look identical to the ones in next/public/assets/images and they are pretty large files which would significantly slow down the git clone process
  2. Could we write a section in readme to explain when and how one will update the screenshot script if needed

Could we write a section in readme to explain when and how one will update the screenshot script if needed

Here's the README of how to add and when to update screenshots.

oh great! sorry i didn't look at the readme in docs/next.

what about the generated then? are we fine to drop them when committing to git?

In D5225#154411, @yuhan wrote:

oh great! sorry i didn't look at the readme in docs/next.

what about the generated then? are we fine to drop them when committing to git?

This is a good question. I couldn't think of any down side to drop the images. It took the same time to generate new images as to just modify/update them.

I agree that we should drop them.

Delete generated images to save space

Again, thank you for putting so much effort here! and sorry about keeping you waiting for so long.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 22 2020, 3:05 AM
This revision was automatically updated to reflect the committed changes.