Skip to content

Added regression guard for XSD schema caching - #15

Draft
HassanAkbar wants to merge 4 commits into
mainfrom
feat/perf-spec
Draft

Added regression guard for XSD schema caching#15
HassanAkbar wants to merge 4 commits into
mainfrom
feat/perf-spec

Conversation

@HassanAkbar

Copy link
Copy Markdown
Member

Adds the missing regression spec for XSD schema memoization, and removes an existing example that proved nothing.

The caching already worked. What was missing was anything stopping someone breaking it.

The deleted example asserted the cache "does not grow". Every broken cache also does
not grow, so it passed against no caching at all, a cache that ignores the version key,
and one that reloads on every call.

Four examples, each the sole catcher of a distinct defect:

  1. builds one schema per distinct version
  2. hands back the schema cached under the requested version
  3. keeps exactly one entry when the same version is looked up again
  4. reuses that entry when the version is auto-detected

Known and accepted:

  • No timing assertion anywhere. The spec proves expensive work is skipped, never that
    anything is fast.
  • It reaches into SCHEMA_CACHE and calls a private method. A cache-behaviour guard
    has to touch the cache it guards.
  • An around hook snapshots and restores the cache.
  • Thread safety is out of scope. SCHEMA_CACHE[version] ||= ... is not atomic. That is
    duplicated work, not corruption, and fixing it means changing lib/.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a dedicated regression spec to guard Dcc::Validate::Xsd schema memoization behavior, replacing a prior example that didn’t meaningfully constrain caching correctness.

Changes:

  • Add spec/dcc/validate/xsd_performance_spec.rb with four examples asserting per-version schema caching and reuse (including :auto).
  • Remove the previous “caches the loaded schema” example from spec/dcc/validate_spec.rb.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
spec/dcc/validate/xsd_performance_spec.rb New regression coverage for XSD schema cache behavior (build count, keying, reuse, :auto).
spec/dcc/validate_spec.rb Removes an ineffective cache assertion in favor of the dedicated regression guard.
Suppressed comments (2)

spec/dcc/validate/xsd_performance_spec.rb:56

  • eq uses ==, which may not guarantee object identity. Since this example is intended to guard memoization, it should assert that schema_for returns the exact same object instance stored in SCHEMA_CACHE (via be/equal).
    looked_up = versions.to_h { |v| [v, described_class.send(:schema_for, v)] }

    expect(looked_up).to eq(versions.to_h { |v| [v, cache.fetch(v)] })

spec/dcc/validate/xsd_performance_spec.rb:47

  • This assertion makes the spec order-dependent on cache.keys (insertion order). The behavior being guarded is “which versions are cached”, not key ordering, so this can be made more robust by asserting count and keys separately.
    expect([builds.size, cache.keys]).to eq([2, %w[3.3.0 3.2.0]])

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread spec/dcc/validate/xsd_performance_spec.rb Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

spec/dcc/validate/xsd_performance_spec.rb:59

  • This example uses send to call the private schema_for method. The repository contribution guidelines explicitly forbid using send to call private methods (see CONTRIBUTING.adoc:34). You can assert the same behavior via the public .call API by instrumenting the cached schema instances and checking that each version’s cached schema receives exactly one #validate call when that version is requested.
  it "hands back the schema cached under the requested version" do
    versions = %w[3.3.0 3.2.0]
    versions.each { |version| described_class.call(xml, version: version) }

    looked_up = versions.to_h { |v| [v, described_class.send(:schema_for, v)] }

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants