Skip to content

Split reconciler spec into per-concern files - #43

Draft
HassanAkbar wants to merge 3 commits into
mainfrom
chore/todo-11-split-reconciler-spec
Draft

Split reconciler spec into per-concern files#43
HassanAkbar wants to merge 3 commits into
mainfrom
chore/todo-11-split-reconciler-spec

Conversation

@HassanAkbar

Copy link
Copy Markdown
Member

Metanorma PR checklist

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

This PR continues Uniword’s ongoing effort to make the DOCX reconciler specs easier to navigate and run by splitting a large monolithic spec/uniword/docx/reconciler_spec.rb into smaller, per-concern spec files under spec/uniword/docx/reconciler/.

Changes:

  • Moves multiple reconciler concerns into new per-concern spec files (notes, tables, parts, body, package_structure).
  • Trims spec/uniword/docx/reconciler_spec.rb down to the remaining top-level/orchestration concerns.
  • Updates the internal TODO note to reflect current sizing/ownership and the intended split strategy.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
TODO/11-split-reconciler-spec.md Updates the planning note with current size metrics and a module-owner-based split plan.
spec/uniword/docx/reconciler_spec.rb Removes extracted concerns, leaving the remaining root/orchestration specs.
spec/uniword/docx/reconciler/body_spec.rb New spec file covering body-related reconciliation behaviors (value preservation; headers/footers).
spec/uniword/docx/reconciler/notes_spec.rb New spec file covering notes creation/validation/reordering behaviors.
spec/uniword/docx/reconciler/package_structure_spec.rb New spec file covering “Group 3” package consistency rules.
spec/uniword/docx/reconciler/parts_spec.rb New spec file covering numbering reconciliation behavior.
spec/uniword/docx/reconciler/tables_spec.rb New spec file covering table reconciliation and gridAfter behavior.
spec/uniword/docx/reconciler/referential_integrity_spec.rb Extends the existing referential integrity spec with additional extracted examples.
Comments suppressed due to low confidence (2)

spec/uniword/docx/reconciler/body_spec.rb:37

  • Uniword::Wordprocessingml::Text stores its text in the content attribute (see lib/uniword/wordprocessingml/text.rb). Using value: here doesn’t set the mapped content and can make the test data effectively empty. Use content: to match the model.
      run.text = Uniword::Wordprocessingml::Text.new(value: "Hello")

spec/uniword/docx/reconciler/referential_integrity_spec.rb:293

  • Uniword::Wordprocessingml::Text maps element content to the content attribute. Using value: here doesn’t match the model and can leave the text empty, weakening what this example is actually exercising.
        text: text_class.new(value: "text"),

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

Comment thread spec/uniword/docx/reconciler/referential_integrity_spec.rb Outdated
Comment thread spec/uniword/docx/reconciler/body_spec.rb
Comment thread spec/uniword/docx/reconciler/package_structure_spec.rb
Comment thread spec/uniword/docx/reconciler/parts_spec.rb
Comment thread spec/uniword/docx/reconciler/notes_spec.rb

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 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

spec/uniword/docx/reconciler/notes_spec.rb:595

  • This example is meant to verify reordering by body reference order, but with the default (non-builder-managed) path the reconciler renumbers note IDs sequentially after reordering. That makes user_entries.map(&:id) end up ["1", "2"] even if the reorder step regresses, so the spec doesn’t actually assert the behavior it describes. Consider running this example with builder_managed: true so IDs aren’t renumbered, and then assert the reordered IDs match the body’s first-reference order.
      described_class.new(package).reconcile

      user_entries = footnotes.footnote_entries.reject { |e| %w[separator continuationSeparator footnoteSeparator continuationNotice].include?(e.type) }
      expect(user_entries.map(&:id)).to eq(%w[1 2])
    end

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 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

spec/uniword/docx/reconciler/referential_integrity_spec.rb:268

  • The explanatory comment here contains several >80-char lines, which violates the repo’s line-length convention and can trigger RuboCop Layout/LineLength failures. Please wrap the comment so each line stays within the configured max.
# Separate top-level group on purpose: these merged-in examples need
# `described_class`, while the group above must keep its exact
# "Reconciler referential integrity" description so `rspec -e` filters and CI
# configs keep matching it. Merging the two back would silently break that
# filter — no test would fail to tell you.

spec/uniword/docx/reconciler/notes_spec.rb:594

  • This example is named/commented as a reorder-by-reference-order test, but the assertion only checks the (renumbered) footnote IDs, which will still be sequential even if the reordering step is broken. Add an assertion on the body’s footnote reference IDs so the spec actually fails when reorder-by-reference does not occur.
      user_entries = footnotes.footnote_entries.reject { |e| %w[separator continuationSeparator footnoteSeparator continuationNotice].include?(e.type) }
      expect(user_entries.map(&:id)).to eq(%w[1 2])

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