Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Simplify the process of creating baselines using Kaleido and improve image & other export test systems #5724

Merged
merged 59 commits into from
Jun 25, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jun 10, 2021

Closes #5625
Supersedes #3237 and #2615 🎉 🎉

Bye bye testbed image server and possibly npm run docker -- run (for ubuntu users)
Hello Kaleido!

@jonmmease
@plotly/plotly_js

Please review commit by commit.

TODOs:

  • investigate if the diffs may be reduced by using exact fonts used to be applied by testbed
  • update image-test readme

@archmoj
Copy link
Contributor Author

archmoj commented Jun 11, 2021

TODOs:

  • investigate if the diffs may be reduced by using exact fonts used to be applied by testbed

OK. For that we need to install googleFonts.
Let me push the changes.

Comment on lines +93 to +94
name: install kaleido v0.2.1
command: python3 -m pip install kaleido==0.2.1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: install kaleido v0.2.1
command: python3 -m pip install kaleido==0.2.1
name: install kaleido
command: pip install kaleido==0.2.1

again, in this env we should only have one pip to worry about... also let's not repeat the version number. (applies several places below too)

# install required fonts (if missing) on ubuntu
sudo cp -r .circleci/fonts/ /usr/share/ && sudo fc-cache -f
# upgrade pip (if needed)
python3 -m pip install --upgrade pip
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear re: my earlier comments - here the instructions are for local use, so using the longer python3 -m pip syntax is reasonable as a way to indicate to users that it's important to do this with Python 3 if nothing else :)

# 'old' image server and we use orca
#
# having problem creating baselines for 2 mapbox mocks using kaleido
# we must use orca
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other mapbox mocks work? what happens with these two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
// mapbox behave differently from run-to-run
// skip error on mapbox diff
if(mockName.substr(7) === 'mapbox_') continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, so we're including mapbox images in these tests, and generating diffs from them, but not allowing those diffs to fail? When do we actually look at these diffs then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a look at diffs during the release cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add a note here after https://github.com/plotly/dev-docs/pull/188 and this PR merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To filter mapbox mocks this should be substr(0, 7) not substr(7).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes are in b1de0bb.
Thanks to @antoinerg comment, now we test everything with zero tolerance except mapbox_* and gl3d_bunny-hull which are tested with certain tolerance.

@archmoj
Copy link
Contributor Author

archmoj commented Jun 24, 2021

Next we should possibly drop Balto Droid_Sans Droid_Sans_Mono Droid_Serif Gravitas_One from our tests and use some other google-fonts instead that are not deprecated in the Google Fonts API.
We could also download OpenSans fonts from the github source; however considering a potential big diff, we may decide to update that one in a separate PR.

@archmoj
Copy link
Contributor Author

archmoj commented Jun 24, 2021

Next we should possibly drop Balto Droid_Sans Droid_Sans_Mono Droid_Serif Gravitas_One from our tests and use some other google-fonts instead that are not deprecated in the Google Fonts API.
We could also download OpenSans fonts from the github source; however considering a potential big diff, we may decide to update that one in a separate PR.

OK. I found GravitasOne.
We now use Nato* instead of Droid* and Roboto instead of Balto in our tests.

The only remaining ttf files are OpenSans which I could live with them.
But I added a download script for them and simply comment that out for now.
🙏 @alexcjohnson almost time to dancer this PR please.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Let's do it!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace old image test system with Kaleido
2 participants