Add a random sample utility

[zachmargolis]

I happened to look at #551 and saw that we were copy-pasting a pattern used many times that is arr[Math.floor(Math.random() * arr.length]) and I figured, what a great opportunity to Not Repeat ourselves.

Hubot had a msg.random helper so I modeled this after that. Open to renaming it as well

If we like this, I am happy to update the wiki to suggest it! Or add a lint if we want too.

I'm also looking for help with the test suite, some of the cases seem to be returning undefined in ways that I don't expect and my attempts to print-debug were not successful

---

Checklist:

• Code has been formatted with prettier
• The OAuth wiki
  page has been updated if Charlie needs any new OAuth events or scopes
• The Environment Variables
  wiki page has been updated if new environment variables were introduced
  or existing ones changed
• The dev wiki has been updated, e.g.:
  • local development processes have changed
  • major development workflows have changed
  • internal utilities or APIs have changed
  • testing or deployment processes have changed
• If appropriate, the NIST 800-218 documentation has been updated

[mgwalker]

I think you'll need to replace Math.random mocks with sample mocks. Everything imported/exported by the utils/test.js module is mocked, and since it's pulling in sample, then where the code-under-test is using sample, it's not getting a value back because the mock isn't... well... mocked.

So for example, the random-response.test.js file has this:

const random = jest.spyOn(global.Math, "random");

...

random.mockReturnValue(0);

This would likely be replaced with something like:

const {
  axios,
  getApp,
  utils: { cache, sample },
} = require("../utils/test");

...

sample.mockReturnValue({ ...the object that should be returned from the list of options });

Alternatively, you could remove sample from the test utilities, I think. Mocking the sample method means changing all those .mockReturnValue(0) to .mockReturnValue({ object }). Part of me thinks, "gosh that's a lot of extra work" while another part of me thinks "gosh that makes the tests so much more explicit."

Either way you decide to go, this is a nice addition!

[zachmargolis]

I think you'll need to replace Math.random mocks with sample mocks. Everything imported/exported by the utils/test.js module is mocked, and since it's pulling in sample, then where the code-under-test is using sample, it's not getting a value back because the mock isn't... well... mocked.

Ok thanks! I removed from utils/test.js and am still getting some errors I got before, where it looks like the sample function just returns undefined everywhere? Such as in pugs.test.js which does no explicit mocking of random or anything, any pointers for how to trace that one down? Is there another level of mocking/stubbing of dependencies going on?

[zachmargolis]

I think you'll need to replace Math.random mocks with sample mocks. Everything imported/exported by the utils/test.js module is mocked, and since it's pulling in sample, then where the code-under-test is using sample, it's not getting a value back because the mock isn't... well... mocked.

Ok thanks! I removed from utils/test.js and am still getting some errors I got before, where it looks like the sample function just returns undefined everywhere? Such as in pugs.test.js which does no explicit mocking of random or anything, any pointers for how to trace that one down? Is there another level of mocking/stubbing of dependencies going on?

Ok so I realize it's this line:

charlie/src/utils/test.js

Line 17 in 8539c12

jest.mock("./index");

So now anything that does a require like this:

const  { sample } = require('./utils');

will get a function that just returns undefined, would it be bad if I updated scripts to import directly, like

const sample = require('./utils/sample');

and avoid it getting mocked out like that?

[mgwalker]

I think we can mark the CodeQL failure as a false positive. There's nothing security-sensitive about how we're using randomness here.

[zachmargolis]

I think we can mark the CodeQL failure as a false positive. There's nothing security-sensitive about how we're using randomness here.

Thanks! I think all the code is ready for final review now, got specs passing.

[mgwalker]

[mgwalker]

To your question in the PR, this seems fine! I don't think it's my ideal case, but it's better than fighting with the tests forever. Maybe someday we can come back and find a way to make the tests work properly, but this is totally within the realm of "good enough," which is all anything needs to be. 😂

[mgwalker]

omg I love this so much more.

[mgwalker]

I had to double-check when default arguments are evaluated. I think in Python they're evaluated at definition time, in which case every invocation of sample would get the same value. But you're correct, in Javascript they're evaluated at call time. 🎉

[mgwalker]

🤩