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

Imbalance in function source opening/closing shown in markdown/html reports #2208

Closed
ScottFreeCode opened this issue Apr 14, 2016 · 1 comment

Comments

@ScottFreeCode
Copy link
Contributor

This is a bit of an edge case since normally test functions are not named (in the sense of the function itself having a name) and one does not usually compress the body of the function due to readability/structure, but for what it's worth, this code...

var assert = require("assert")

it("has the head of the function but not the closing brace", function opening() {
    assert(true)
})
it("does the same thing with a function declared elsewhere", functionDeclaredOutside)
it("has the closing brace of the function but not the head", function () {assert(true)}) // a space before the parens doesn't affect it, must be the name
it("does not have the closing brace because of just one space after the contents", function() {assert(true) })
it("has the closing brace if the function body is entirely empty", function()
    {}
)
it("does not have the closing brace because of just one space in the body",function(){ })

function functionDeclaredOutside() {
    assert(true)
}

...yields this output when run with the markdown reporter...


has the head of the function but not the closing brace.

function opening() {
    assert(true)

does the same thing with a function declared elsewhere.

function functionDeclaredOutside() {
    assert(true)

has the closing brace of the function but not the head.

assert(true)}

does not have the closing brace because of just one space after the contents.

assert(true)

has the closing brace if the function body is entirely empty.

}

does not have the closing brace because of just one space in the body.


I would guess that the inclusion of function name() { in named functions is intentional, so that references to the name from within the function are clear; but in any case the closing brace being included or not should match whether the function name/opening is included, rather than whether there's whitespace before it.

@ScottFreeCode
Copy link
Contributor Author

On further reflection, I don't think the inclusion of function name() { is intentional:

  1. What are you going to do with a reference to the test function when all test functions are the same except for presence or absence of the done parameter?
  2. It would be more helpful to still see the done parameter in case it was named something different; but that's not being shown either unless the function is named as far as I can tell, so we're obviously not that concerned that people see the function declaration in the one case where it actually could (in unlikely circumstances) matter. Although I suppose if we wanted to we could skip stripping if the name of the parameter isn't done, but that would basically be a different sort of change from fixing the discrepancy described by this ticket.
  3. I found another error, that it strips up to the last ) { or ){ if you have an if or a nested function on the same line as the opening of the function (I know, don't write real tests like this; I was just trying to run a quick experiment to explore Mocha's functionality...), rather than only the initial function() {. This strongly suggests a regex error, specifically an overly broad character class (or a dot) in part of the pattern that's supposed to match the initial function declaration before the {. If there are regex errors, it's entirely possible that another one is using function[(] (which would not match named functions) instead of function[^(]*[(] (which would).

So, I ended up finding where this is being cleaned and it certainly looks like my regex error theory; I plan on submitting a pull request to tweak it and correct the various oddities.

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

No branches or pull requests

1 participant