Overall I would love to have more unit and integration tests, maybe even some screnshot comparison to understand if the CSS is breaking something.

Regarding this examples specifically:

• I would like to keep the testing libraries to only one, since we already have mocha we should keep it or update the remaining tests if other library seems more suitable.
• The testing library should only be available as a DevDependency (should not be needed for runtime)
• This project was how I learned JS so the code is not very good (maybe even quite bad 😞) for testing. In my daily work I really try to avoid mocks and rewrite the code to be more injectable so we can test most of it just by passing the needed parts.

In sum I have two questions:

• What is your experience with sinon? (I don't do JS daily to I am not following the trends/innovations) Do you think switching would be interesting?
• About the tests what do you think about using Dependency Injection / Method Passing instead of mocks for the majority of the tests?

Mocha does not come equipped with spies and the library recommends Sinon.

Spies are particularly useful for unit testing. If the code is modular (and especially if it's functional) it's enough to test that function calls other functions / methods with appropriate arguments. For that we need to create a spy / stub.

Sorry I was tired and accidentally stuck sinon in Dependencies not DevDependencies 😓

At work we use Jasmine which includes spies and better logging when expected value is different from actual. It's easy to work with and given how few written tests there are, it would likely be simple to switch over.

I am more eager to get #42 to a state that's worth merging; we could revisit this PR after then 👍

I've barely ever used Sinon - just a bit here: https://github.com/whyboris/karma-helpful-reporter
I've not done much research so I'm unsure about the quality of anything other than Jasmine

I'm sorry I'm unsure what you mean with "Dependency Injection / Method passing" with respect to tests. Do you mean just to use the actual Diff2HtmlInterface and see if all the pieces work together by "expecting" correct output given standardized input?

@whyboris my idea was going more in this direction:

• https://hernantz.github.io/mock-yourself-not-your-tests.html
• https://philippe.bourgau.net/avoid-mocks-and-test-your-core-domain-faster-with-hexagonal-architecture/#
• https://www.artificialworlds.net/blog/2014/04/11/avoid-mocks-by-refactoring-to-functional/

but maybe this is not that important in this small application. If you feel like contributing some tests I will be ok accepting the mocks. Also if you think jasmine would be better feel free to change. I have nothing special regarding mockito.

@whyboris if you change the dependency to devDependencies I can merge this one.

I'll look into failing tests tomorrow 👌 unsure what happened yet.

Thank you for the links -- good reading material!

I might try hooking up Jasmine just to see how it goes 👍

edit: the failing tests were because we now have the ignore argument in the getInput method. Fixed with cdabb61

Sorry for so many commits, please consider Squash and merge rather than merge commit whenever this PR is ready 😅

Currently we're using Istanbul which is no longer maintained and NYC is recommended instead.

Simply replacing istanbul with nyc in package.json and switching the coverage script to nyc mocha we get this:

image

Would you like me to make the switch in this PR ? :)

This is what the change for NYC would look like: whyboris@bfbee97

It should keep all the functionality the same. As a bonus it will include a separate command for generating an HTML output npm run coverage-html 👍

Would you like me to make the switch in this PR ? :)

Go ahead 👍

node 5 fails with:

> nyc mocha
/root/diff2html-cli/node_modules/nyc/node_modules/find-up/index.js:5
module.exports = (filename, opts = {}) => {
 ^
SyntaxError: Unexpected token =

Will not work with nyc I suspect

node 4 fails with:

The package ajv@6.8.1 does not satisfy its siblings' peerDependencies requirements!

Fails during npm install
ajv seems to be part of eslint - perhaps other dependencies have it as a subdependency, and an early version of npm can't handle the different version complexity? Unsure why it passed before but not now otherwise.

Now both node4 and node5 fail with the Unexpected token = in the nyc library.

It looks like #44 PR will likely require abandoning earlier node versions anyway; do you think it would be acceptable / appropriate to call the next version a major release, dropping support for all node versions prior to 8?

The Node timeline suggests that everyone should be at least on version 8 by April 2019 (node 6 leaves maintenance status then).

@whyboris sure. That sounds appropriate.

We could also add 11 to the test list.

Great 👍 -- please let me know if there's anything I should do with this PR for you to merge it.

Of course I don't mean to hurry you - no rush on any of this 😁

Remove the old node versions and add 11. That should be enough.

😅 I hadn't realized that the configuration for this was in the .yml file 👍