Fix noise() getting overridden; add tests

[davepagurek]

Resolves #8026

Changes:

• Removes a line that double-adds noise as a p5.strands function, causing it to not work outside of p5.strands
• Adds integration tests for regular noise and strands noise

Postmortem

There actually were existing noise unit tests in src/test/unit/noise.js. However, they directly imported the noise function from that file and don't run all of p5. So functionality in p5.strands that conditionally overrides other p5 functions to make them work in shaders never gets run in the unit test.

As a workaround right now, for testing all of p5, I've added a visual test that uses noise. Before the line removal in ShaderGenerator.js these visual tests looked broken; afterwards, they look correct. So this particular issue won't happen again.

However, we still have an opening for similar issues in the future, as p5.strands overrides and wraps existing function implementations, and any test file that just imports that function and not all of p5 will not actually be testing the overrides. I haven't done any of these here, but a few paths forward are:

• Test through full p5 to mirror how functions get called in users' sketches. The benefit is that we have good test coverage generally, so this would catch such errors, at the cost of having tests be less self-contained and quick to run.
• In general, make separate integration tests from unit tests so functions are also called through the rest of p5. This also works, and keeps unit tests light, but also adds a lot more tests we need to write, and it's not super clear what should go in just a unit test or just an integration test, so I'm not sure it's actually lighter overall?
• Add unit tests specifically for p5.strands overrides. These would need to test both the in-strands and out-of-strands cases through p5 and not through direct imports. This is probably the better long term approach but it means that if we do similar overriding elsewhere in p5 (e.g. in FES to add parameter validation) we would in theory need to similarly make a bunch of tests of everything overridden, which is kinda heavy and easy to overlook since wrapping/overriding to add functionality is slightly encouraged.
• Move overriding out of p5.strands and into each individual function. This means errors will get caught in existing unit tests, and it's super clear to contributors how data flows through functions in p5 since it's now explicit at the function definition. However, it means p5.strands is no longer encapsulated, and the rest of the codebase is made aware of it.

Anyone else have thoughts? cc @limzykenneth

[davepagurek]

This is the actual bug fix, the rest here is to make tests work

[davepagurek]

This is a fix for an issue I ran into when making tests, where passing a p5 instance variable with the name p5 as context for a strands callback breaks internal functionality by overriding the p5 name

[davepagurek]

aaaand for a reason I don't understand yet, the shader noise is looking different locally vs ci 🤔

Local:

CI:

Not sure why yet, possibly due to noise relying on calculations with lots of decimals and amplifying the decimals of the result? At least both look like noise, so I'm just going to comment out the shader noise check so we can merge the fix in, but I'll add this to the to do list of weird CI things to investigate

[limzykenneth]

I think I would much prefer the second option of "In general, make separate integration tests from unit tests so functions are also called through the rest of p5" that separate unit tests and integration tests. One of the benefit for individual import unit test (although not yet fully realized) is the test rerun can only rerun changed files instead of rerunning everything, making local development a bit more streamlined. With importing p5 fully, any changes anywhere will rerun all tests which is quite slow for dev feedback loop.

This also ties in a bit with the goal of custom builds with individual module, we want to keep things as independent from each other as possible, which also means noise() should not need to know p5.strands exist and p5.strands should handle the interaction with native noise() in both its implementation and test if possible (otherwise in integration test).

[davepagurek]

I guess visual tests are already a fairly convenient way of doing integration tests since most fully-integrated p5 stuff is visual in nature, and we can always make new regular test files for non-visual integration tests?