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.
Details
Details
- Reviewers
• max • yuhan • sashank - Commits
- R1:ae52e8659206: Fix schreenshots test: Part 1
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
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
| 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 | |
| 272 | 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 | |
| 275 | same as above | |
| 738 | can you get rid of this sleep and wait for something specific? or change the waitUntil param on the previous call? | |
| 739 | maybe better to have a long timeout on this (10 seconds or sth) rather than hang forever if it fails | |
| 760 | an you wait for something specific on the new tab instead? | |
| 998 | can you wait for something specific instead? | |
| 1019 | can you wait for something specific instead? | |
| 1450 | can you wait for something specific instead? | |
| docs/next/public/assets/images/tutorial/complex_pipeline_figure_one.png | ||
| 1 | i think we should probably ignore this directory for diffs -- | |
| 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. | |
| docs/generate_screenshots.test.js | ||
|---|---|---|
| 272 | 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. | |
Comment Actions
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. | |
| docs/next/public/assets/images/tutorial/complex_pipeline_figure_one.png | ||
|---|---|---|
| 1 |
We should only review images under docs/next/public/assets/images/tutorial. Snapshots are hidden by set the config of Phabricator . | |
Comment Actions
Update: Clean sleep for n secs. Some unnecessary waitings are removed and some are changed to wait for specific selector.
Comment Actions
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
Comment Actions
Update: Fix inputs_figure_seven.png having double config error messages after going through all the images in the tutorials
Happy weekend!
Comment Actions
This looks good to me! Thanks for doing this.
Two comments before accepting this:
- 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
- Could we write a section in readme to explain when and how one will update the screenshot script if needed
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
Again, thank you for putting so much effort here! and sorry about keeping you waiting for so long.