Separate parsing of test results from report generation#1398
Separate parsing of test results from report generation#1398chrisdarroch wants to merge 8 commits intocompat-table:gh-pagesfrom
Conversation
…ation. - rename testScript function to prepareTest - remove use of cheerio from prepareTest function - introduce wrapWithScript function that converts return value of prepareTest in to an executable function in the DOM these changes allow for the data parsing behaviour of the build module to be extracted and separated from the page report generation behaviour.
every script tag now has a newline at its end.
build.js
Outdated
| function dataToHtml(skeleton, rawBrowsers, tests, compiler) { | ||
| var wrapWithScript = function(assertions) { | ||
| // wrap a single test in an array so we can deal with them consistently | ||
| if (assertions && !Array.isArray(assertions)) { |
There was a problem hiding this comment.
If you’re checking isArray, you could just do [assertions]; concat is how you’d do it without the if check at all.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
build.js
Outdated
| } | ||
|
|
||
| function dataToHtml(skeleton, rawBrowsers, tests, compiler) { | ||
| var wrapWithScript = function(assertions) { |
There was a problem hiding this comment.
This function can be hoisted out of dataToHtml
There was a problem hiding this comment.
I agree that it could, but there's another five functions that exist below it within the dataToHtml closure that don't need to be there, either. I was aiming for consistency. Should I move all of them in a separate refactor, perhaps?
| @@ -0,0 +1,111 @@ | |||
| // Copyright 2012 Dan Wolff | danwolff.se | |||
There was a problem hiding this comment.
Why 2012 and who is copyright owner?
There was a problem hiding this comment.
I split the original file in two, preserving history. Though github implies this is new content, it is in fact unchanged since the original file was created.
The copyright headers in both files could be revised, though I feel it is a separate cleanup task unrelated to this PR's scope.
|
Is there anything further I can do to get this PR merged? |
| var $ = cheerio.load('' | ||
| + '<script' + (assertion.type ? ' type="' + assertion.type + '"' : '') + '>' | ||
| + assertion.script | ||
| + '\n</script>' |
There was a problem hiding this comment.
\n added here, but not after <script>. It's inconsistency. It should be in both places or nowhere. The rest LGTM.
There was a problem hiding this comment.
It’s needed here for single-line comments; it’s not needed after the closing tag. So no matter what, it can’t be nowhere. I’m in favor of including it after as well, tho.
There was a problem hiding this comment.
I mean after opening tag. At this moment, it's nowhere and it's the reason of changes in generated HTML.
There was a problem hiding this comment.
I only see one before the closing tag - but sure, fair enough.
There was a problem hiding this comment.
Apparently, we just did not understand each other. It's added before the closing tag and it's the reason of changes in generated HTML, but not after opening tag. Adding it in both places could make sense.
4 participants
This refactor makes it simpler to consume this repository as a set of tests and a test runner for the various JS environments. The
build.jsmodule remains responsible for constructing the compatibility table, where thetest-parser.jsmodule is responsible for prepping individual assertions for checking in a given environment.I've done what I can to preserve the git history after splitting the files so that
git blamecontinues to work in both files.