Skip to content

Maintainable tests of actual SVG output #47

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

Merged
merged 15 commits into from
May 5, 2016

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Apr 13, 2016

This is the regression test suite I'm using to work on #46 confidently.

For ease of maintenance, I included a script (bootstrap-tests.js / npm run test:bootstrap) that can be used to update the reference renderings in-place when they legitimately change.

That script is kind of a lot of code and probably not the prettiest, but I hope you'll find it good enough to accept as-is. I'll attach some more notes to the relevant commits in a moment.

motiz88 added 10 commits April 13, 2016 16:16
(Run bootstrap-tests.js with e.g. babel-node to generate a new fixtures.js)
Importantly, we normalize property order in the SVG style attribute before comparing, to avoid spurious errors on essentially identical images.
A side benefit is nicer error messages on test failures.
bootstrap-tests.js relies on the particulars of this module's string formatting behavior, so let's be defensive here.
const htmlCode = JSON.stringify(wrapper.html());

for (let dataKey of Object.keys(recognizedDataStrings)) {
jsxCode = replaceAll(recognizedDataStrings[dataKey], dataKey, jsxCode);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@borisyankov This allows us to write data={sampleData} in fixtures.js rather than data={[0.26789611283279424, ...]} every time. Yes, I used string replacement to do this. It's slow and kind of inelegant but it works :) Which to my taste is good enough for this kind of dev script.

@borisyankov
Copy link
Owner

I see this PR also contains the changes from the other PR fixing Travis... they both did get a merge conflict caused by my last commit.
Maybe fix just this one, and I'll merge it.

@motiz88
Copy link
Contributor Author

motiz88 commented Apr 14, 2016

I've now updated all my PRs with your latest work on master.

@borisyankov borisyankov merged commit 2c3cea9 into borisyankov:master May 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants