Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add instructions for image-based tests to the contributors guide #6073

Merged
merged 9 commits into from Feb 25, 2019

Conversation

@benmccann
Copy link
Collaborator

commented Feb 17, 2019

This is mostly borrowed from #3988 along with incorporating some info from recent questions asked in the Slack channel

@benmccann benmccann force-pushed the benmccann:image-tests branch from ac45be1 to bfc55fa Feb 17, 2019

@nagix
Copy link
Collaborator

left a comment

This is very helpful for new contributors!

Show resolved Hide resolved docs/developers/contributing.md Outdated
Show resolved Hide resolved docs/developers/contributing.md Outdated
Show resolved Hide resolved docs/developers/contributing.md Outdated
Show resolved Hide resolved docs/developers/contributing.md Outdated
Show resolved Hide resolved docs/developers/contributing.md Outdated

@simonbrunel simonbrunel added this to the Version 2.8 milestone Feb 20, 2019

@benmccann benmccann dismissed stale reviews from nagix, kurkle, and etimberg via 9fc7b57 Feb 20, 2019

@kurkle
Copy link
Collaborator

left a comment

Couple of things that sound wrong to me. I also noticed that debug is not needed for new tests, because its going to fail anyway when the png is missing. Only really need debug when trying to figure out why some test doesn't fail, right?

Show resolved Hide resolved docs/developers/contributing.md Outdated
Show resolved Hide resolved docs/developers/contributing.md Outdated
@benmccann

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2019

You're right. I just tested and debug doesn't seem necessary. I removed that step from the instructions

@Vincent-Ip

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Hi @benmccann, I was using this updated guide to add an image test for a new feature. This guide was pretty easy to follow. I did have just some minor comments to add to this PR. Hopefully they are helpful.

benmccann added some commits Feb 22, 2019

@kurkle

This comment has been minimized.

Copy link
Collaborator

commented Feb 24, 2019

I thin the debug option should still be documented.

@benmccann

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 24, 2019

Ok. Added a line about the bug option

Show resolved Hide resolved docs/developers/contributing.md Outdated

@benmccann benmccann force-pushed the benmccann:image-tests branch from 04d2566 to cdb1b9a Feb 25, 2019

@kurkle

kurkle approved these changes Feb 25, 2019

@nagix

nagix approved these changes Feb 25, 2019

@simonbrunel
Copy link
Member

left a comment

Thanks @benmccann, I should have wrote these guidelines long time ago :)

@simonbrunel simonbrunel merged commit 79fc340 into chartjs:master Feb 25, 2019

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.622%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.