-
Notifications
You must be signed in to change notification settings - Fork 943
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
Separate parsing of test results from report generation #1398
base: gh-pages
Are you sure you want to change the base?
Separate parsing of test results from report generation #1398
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, i like the direction!
build.js
Outdated
@@ -397,6 +399,25 @@ function handle(options) { | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -397,6 +399,25 @@ function handle(options) { | |||
} | |||
|
|||
function dataToHtml(skeleton, rawBrowsers, tests, compiler) { | |||
var wrapWithScript = function(assertions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be hoisted out of dataToHtml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that's fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
@@ -0,0 +1,111 @@ | |||
// Copyright 2012 Dan Wolff | danwolff.se |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 2012 and who is copyright owner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see one before the closing tag - but sure, fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.js
module remains responsible for constructing the compatibility table, where thetest-parser.js
module 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 blame
continues to work in both files.