Add multipart/form-data unit tests

[honzajavorek]

Ported from Dredd: apiaryio/dredd@3d396f6

[michalholasek]

As I mentioned in sister PR comment, I think it would be good to emphasize that these are private/internal functions by leading underscores for example.

[honzajavorek]

Airbnb prohibits underscores, so we'd need ESLint exception for that.

I decided to implement the latest Airbnb JS filename conventions in this new code, so people reading the code should only see something like:

const getFooBar = require('./getFooBar');
getFooBar(42);

I hope such interface could help to make it clear the file does one thing and that one thing is required/imported. In the context of unit tests where the individual helpers do get required/imported, the intention could be still quite clear as the reader is... reading unit tests.

Where the intention isn't quite clear is the exports code itself. One solution could be a simple comment:

}


// for the purposes of unit tests
compile.compileBody = compileBody;
compile.hasMultipartBody = hasMultipartBody;


module.exports = compile;

Underscores could be even more explicit, but require some additional work:

}


// for the purpose of unit tests
compile._compileBody = compileBody;
compile._hasMultipartBody = hasMultipartBody;


module.exports = compile;

This also prevents property shadowing in case compile is an object or class, rather than a function. In this case ESLint needs to be silenced though:

}


/* eslint-disable no-underscore-dangle */
compile._compileBody = compileBody;
compile._hasMultipartBody = hasMultipartBody;
/* eslint-enable no-underscore-dangle */


module.exports = compile;

The silly thing is I could not figure out how to add any extra information to the ESLint comment to let the reader know this is for the purposes of unit tests. If I add anything to the comments, they stop to get recognized by ESLint. The only solution would be to add an extra comment, which is really ugly:

}


// for the purposes of unit tests
/* eslint-disable no-underscore-dangle */
compile._compileBody = compileBody;
compile._hasMultipartBody = hasMultipartBody;
/* eslint-enable no-underscore-dangle */


module.exports = compile;

Luckily, at least the imports/requires in unit tests do not need any special treatment regarding ESLint:

const {
  _compileBody: compileBody,
  _hasMultipartBody: hasMultipartBody
} = require('../../src/compile');

A different solution for exports would be maybe to...

// for the purposes of unit tests
compile._compileBody = compileBody; // eslint-disable-line no-underscore-dangle
compile._hasMultipartBody = hasMultipartBody; // eslint-disable-line no-underscore-dangle

Or maybe just this, since it's always going to be at the end of the file?

/* exports for unit tests *//* eslint-disable */
compile._compileBody = compileBody;
compile._hasMultipartBody = hasMultipartBody;

Any thoughts on this? Do we want to do it this way? Is there an elegant solution to this?

[michalholasek]

Personally, I like

// For the purpose of unit tests
compile._compileBody = compileBody;
compile._hasMultipartBody = hasMultipartBody;

the most.

Regarding no-underscore-dangle eslint rule - do we need (want) it though? Just because Airbnb's JavaScript styleguide is considered the holy grail, we don't have to think about it as such (and because of decaff, we have different settings for some rules anyway).

On the other hand, we could (and probably should) think about any change to public contract as breaking - then something like

// For the purpose of unit tests
compile.compileBody = compileBody;
compile.hasMultipartBody = hasMultipartBody;

would be perfectly fine.

[honzajavorek]

I don't like the reasoning in the Airbnb style guide and the ESLint rule, but that's maybe because I'm a pythonista and for us the convention works just fine. They say you should treat everything what's public as public, but I think the reality is we're all consenting adults.

because Airbnb's JavaScript styleguide is considered the holy grail, we don't have to think about it as such

Well, style guides are here to follow and to keep developers aligned despite their different opinions on code. We can express a project-wide opinion, but that, to certain extent, beats the purpose of having a style guide.

because of decaff, we have different settings for some rules anyway

That's an outstanding bug to be resolved 😉 And just FYI, there are no exceptions on this particular repo except one rule you despised and I adored (but I was lazy to fight, even though the style guide was on my side).

---

That said, I like following the most as well:

// For the purpose of unit tests
compile._compileBody = compileBody;
compile._hasMultipartBody = hasMultipartBody;

We can disable the rule on purpose in our projects and if future maintainers disagree with our decision, they can change it. We can link this discussion from a comment in the .eslintrc.js file as a documentation of the decision.

[michalholasek]

It's settled then 👍 🙂

[michalholasek]

Nit: I think that bodyLF/bodyCRLF would be better names.

[michalholasek]

Just small nits, great work 👍

[honzajavorek]

Updated. I'm not going to rebase as the "out-of-date" status is only caused by .npmrc and I'm afraid rebase could destroy references to the discussion we had.