30-seconds-of-code: Testing

[fejes713]

Since @kingdavidmartins implemented testing 🔥 , it was our task to write test cases for each snippet. 🚒
Today I wrote a bunch of test cases for snippets that aren't complicated in terms of the test environment (need to run in browser etc.). While doing so I've found few problems associated with testing. One of them being:

[BUG] Problems with functions that use recursion:

The list of broken functions: gcd, factorial, elo, recursive, deepFlatten, curry, cleanObj, anagrams (I probably missed some of them).

Here is an explanation for gcd and why does it not work:
The current structure of folder in which test is written:
gcd.js tile:

module.exports = (...arr) => {
const _gcd = (x, y) => (!y ? x : _gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};

and gcd.test.js file:

const test = require('tape');
const gcd = require('./gcd.js');

test('Testing gcd', (t) => {
    t.equal(gcd(8, 36), 4, "Calculates the greatest common divis...");
    t.end();
});

In the case mentioned above, the test fails because gcd is not defined in gcd.js. The same happens with every other function that uses recursion. We have to figure out how to solve this problem.

the other problem is much easier one

[BUG] Some snippets repeat with different names:

I've found some snippets share nearly the same functionality, and I think we should squash them into one snippet. For example: deepFlatten, flatten and flattenDepth all do nearly the same thing, so we could squash them into one more useful snippet.

[Chalarangelo]

As far as recursion goes, I think we should leave the module/JS file generation to the module script and let tdd only create new files for snippet tests that do not exist. That way we deal with the issue once and for all.

As far as merging snippets is considered, so far we have followed lodash's implementation and this has caused a few missteps. This should be a new issue to discuss what our golden rule is about where we split a snippet into two. For example flatten and flattenDepth are essentially the same (if you have a default parameter for depth=1 in the second one, they are the exact same). But deepFlatten goes on forever and is implemented slightly differently. So, my opinion is 2/3 of these could be merged. This is a highly complicated issue, as each snippet is different. Let's open an ongoing discussion about these and update guidelines.

[kingdavidmartins]

@fejes713 Nice Catch!! 💯 pushing simple fix.

@Chalarangelo I honestly don't think it's necessary all that needs to be added is snippetName declaration.

Which would look like this

module.exports = gcd = (...arr) => {
const _gcd = (x, y) => (!y ? x : gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};

instead of this

module.exports = (...arr) => {
const _gcd = (x, y) => (!y ? x : gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};

[kingdavidmartins]

Now your/our test @fejes713 and all the like passes!! 😄

[kingdavidmartins]

For example flatten and flattenDepth are essentially the same (if you have a default parameter for depth=1 in the second one, they are the exact same). But deepFlatten goes on forever and is implemented slightly differently. So, my opinion is 2/3 of these could be merged.

@Chalarangelo I agree. I vote for the removal of flatten

[fejes713]

@kingdavidmartins @Chalarangelo me too!

[Chalarangelo]

I'll deal with flatten tomorrow then, if nobody else PRs it.

Closing as resolved in #528. Thanks for the quick fix @kingdavidmartins!

[skatcat31]

That test is valid under nodejs. Tested it in repl

[skatcat31]

I guess I'm confused in what the issue you ran into was?

[atomiks]

@skatcat31 one of the gcd did not have an underscore prefix (the OP mistakenly listed it as having one). So it needs to have the name of the function (gcd with no prefix) in order to work

[skatcat31]

Then for testing listing it as a global is perfectly fine since it's a test environment. However my personal preference would be to make it a local variable to then be made the export on a line below so as to not mutate the global. The fact that we're mutating the snippet structure instead of merely adding a line with the snippet name might be due to the pattern matching though. @kingdavidmartins how hard would it be to transition to this instead of the global method?

const gcd = (...arr) => {
const _gcd = (x, y) => (!y ? x : _gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};
module.exports = gcd

[Chalarangelo]

@skatcat31 That's not a bad idea and looks a bit cleaner. I think we could do that.

On a side note, I never closed the issue and I haven't yet updated flatten to work with a depth, I will do that in a little bit.

[fejes713]

@Chalarangelo I am doing flatten on a new branch flatten-fix

[Chalarangelo]

@fejes713 Alright, I'd suggest we call the updated merged function flatten instead of flattenDepth but essentially keep flattenDepth's functionality with a default depth of 1, so that without a second argument it's a normal flatten.

[fejes713]

Okay, gotcha. Expect PR soon

[kingdavidmartins]

@skatcat31 Just to confirm is there really any benefit to go with your suggested route which is the following. In case I miss something

const gcd = (...arr) => {
const _gcd = (x, y) => (!y ? x : gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};
module.exports = gcd

When compared to the following

module.exports = gcd = (...arr) => {
const _gcd = (x, y) => (!y ? x : gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};

Since the way snippetName.js is set up. No other methods or code should or would implement/add to the file since it will & only contain the snippets code that we need to export.

Like you said

Testing listing it as a global is perfectly fine since it's a test environment. However my personal preference would be to make it a local variable to then be made the export on a line below so as to not mutate the global.

Updating it to do the following

const gcd = (...arr) => {
const _gcd = (x, y) => (!y ? x : gcd(y, x % y));
return [...arr].reduce((a, b) => _gcd(a, b));
};
module.exports = gcd

Honestly wouldn't be hard, however what's the point to change it if it boils down to personal preference and no real added benefit

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.