[p5.js 2.0+ Bug Report]: Missing unit tests for src/core/States.js

[harshiltewari2004]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• WebGPU
• p5.strands
• Build process
• Unit testing
• Internationalization
• Friendly errors
• Other (specify if possible)

p5.js version

dev-2.0 branch(verified on commit e83e9d4)

Web browser and version

N/A — missing test coverage, not a runtime bug

Operating system

N/A — missing test coverage, not a runtime bug

Steps to reproduce this

This is a test-coverage gap I noticed while exploring the dev-2.0 codebase, not a runtime bug — so the standard reproduction steps don't apply.

Background

src/core/States.js defines the States class, which is the core state-diffing mechanism used by push() / pop() across the library. Despite its central role, it has no dedicated unit test file. Its methods (setValue, getDiff, getModified, applyDiff) are exercised only indirectly through other tests.

Proposed addition

I'd like to add a new file test/unit/core/States.js with direct tests for each method, plus one integration-style test covering the push/pop usage pattern.

import { States } from '../../../src/core/States.js';

suite('States', function () {
  test('initialises with provided state', function () {
    const s = new States({ fill: 'red', stroke: 'blue' });
    assert.equal(s.fill, 'red');
    assert.equal(s.stroke, 'blue');
  });

  test('setValue() updates the value', function () {
    const s = new States({ fill: 'red' });
    s.setValue('fill', 'green');
    assert.equal(s.fill, 'green');
  });

  test('setValue() records the original before first modification', function () {
    const s = new States({ fill: 'red' });
    s.setValue('fill', 'green');
    assert.equal(s.getModified().fill, 'red');
  });

  test('setValue() does not overwrite original on repeated calls', function () {
    const s = new States({ fill: 'red' });
    s.setValue('fill', 'green');
    s.setValue('fill', 'blue');
    assert.equal(s.getModified().fill, 'red');
  });

  test('getDiff() returns the modified map', function () {
    const s = new States({ fill: 'red' });
    s.setValue('fill', 'green');
    const diff = s.getDiff();
    assert.equal(diff.fill, 'red');
  });

  test('getDiff() clears #modified after returning', function () {
    const s = new States({ fill: 'red' });
    s.setValue('fill', 'green');
    s.getDiff();
    assert.deepEqual(s.getDiff(), {});
  });

  test('applyDiff() undoes current modifications and replaces #modified', function () {
    const s = new States({ fill: 'red' });
    s.setValue('fill', 'green');
    const outerModified = {};
    s.applyDiff(outerModified);
    assert.equal(s.fill, 'red');
    assert.deepEqual(s.getModified(), {});
  });

  test('applyDiff() with a non-empty prevModified replaces #modified', function () {
    const s = new States({ fill: 'red', stroke: 'black' });
    s.setValue('fill', 'green');
    const outerModified = { stroke: 'black' };
    s.applyDiff(outerModified);
    assert.equal(s.fill, 'red');
    assert.deepEqual(s.getModified(), { stroke: 'black' });
  });

  test('push/pop cycle: applyDiff restores prior state', function () {
    const s = new States({ fill: 'red' });
    const beforePush = s.getDiff();
    s.setValue('fill', 'green');
    assert.equal(s.fill, 'green');
    s.applyDiff(beforePush);
    assert.equal(s.fill, 'red');
    assert.deepEqual(s.getModified(), {});
  });
});

Side observation

While drafting these tests I noticed that applyDiff(prevModified) is somewhat misleadingly named — it doesn't apply the passed-in diff to the current state. It undoes the current modifications (writing the values stored in #modified back to this[key]) and then replaces #modified with the parameter. The parameter name prevModified reflects this — it's the previous #modified state to reinstall, consistent with pop semantics. This works correctly for its actual use case, but the method name took me a moment to parse against the implementation. Flagging only as an observation, not proposing a rename.

Notes

• Verified on dev-2.0 HEAD (commit e83e9d4) — no existing test file at test/unit/core/States.js.
• All test sketches above were traced against the actual implementation in src/core/States.js to ensure they match real behavior.
• Happy to take this on if approved.

[harshiltewari2004]

Hi @ksen0 @davepagurek I will be very happy to make this changes!

[davepagurek]

Hi @harshiltewari2004, feel free to take this on. Are you also interested in renaming getDiff to make it more closely relate to its usage? I'd also maybe suggest a name that implies a mutation, since get methods usually don't change state, but this does -- maybe it's more akin to a pop on an array or something?

[harshiltewari2004]

Hi @davepagurek , thanks! — happy to take the rename on as part of this. You're right that get is misleading for a method that mutates; the pop analogy fits well.
A few options I considered, would love your preference:
For getDiff() (returns #modified and clears it):

popDiff() — directly adopts the pop analogy you suggested.
flushDiff() — common term for "return current contents and reset to empty," used in stream/buffer APIs.
takeDiff() — "take" implies removal in some functional APIs.

For applyDiff(prevModified) (undoes current modifications, then installs the passed-in map):

popTo(prev) — pairs naturally if getDiff becomes popDiff.
revertTo(prev) — describes the behavior from the caller's perspective.
Leaving applyDiff as-is, since the rename of getDiff may already address the bigger naming concern.

My instinct is popDiff / popTo — they share a vocabulary and both signal mutation. But I'd defer to whatever fits best with the rest of the codebase's conventions. Let me know which direction you'd prefer and I'll include the rename in the same PR as the tests.

[davepagurek]

I maybe gravitate to the take and revert or apply to have some element of opposites, so we don't have pop both times.

[harshiltewari2004]

Good point on the opposites — take + apply reads cleanest to me too. getDiff → takeDiff makes the mutation explicit, and applyDiff already describes its role accurately (it applies a new #modified map after rolling back the current one), so I'd suggest leaving that one as-is.
If that works for you, I'll go with takeDiff and keep applyDiff unchanged. Will start on the PR (tests + rename + updating call sites in src/).