diff --git a/TODO/04-reduce-spec-ivar-use.md b/TODO/04-reduce-spec-ivar-use.md index 987c4c3b..29bc7033 100644 --- a/TODO/04-reduce-spec-ivar-use.md +++ b/TODO/04-reduce-spec-ivar-use.md @@ -1,67 +1,118 @@ -# 19 — Reduce spec `instance_variable_set`/`get` (30 sites) - -**Priority:** Medium (spec quality) -**Files:** Multiple spec files: -- `spec/uniword/wordprocessingml/comments_part_spec.rb` (4) -- `spec/uniword/wordprocessingml/tracked_changes_spec.rb` (4) -- `spec/uniword/wordprocessingml/comment_spec.rb` (1) -- `spec/uniword/wordprocessingml/comment_range_spec.rb` (2) -- `spec/uniword/validators/paragraph_validator_spec.rb` (4) -- `spec/uniword/validators/element_validator_spec.rb` (1) -- `spec/uniword/validators/table_validator_spec.rb` (1) -- `spec/uniword/infrastructure/zip_extractor_spec.rb` (5) -- `spec/uniword/builder/run_builder_spec.rb` (1) -- `spec/uniword/builder/image_embedding_spec.rb` (3) -- `spec/uniword/validation/rules/document_context_spec.rb` (1) -- `spec/uniword/validation/link_validator_spec.rb` (3) +# 19 — Reduce spec `instance_variable_set`/`get` -## Problem - -Project rule: never use `instance_variable_set`/`get`. Breaks -encapsulation. +**Status:** Implemented, pending Windows CI. 30 claimed → 6 real → 0. The 6 +were dead code: they set an ivar (`@finalizer`) that `Tempfile` never reads. +Local runs are green, but the workaround existed for a Windows-only `EACCES` +flake, so Windows CI is the gate that closes this out. +**Priority:** Low (the remaining sites are dead code, not encapsulation +breaks) +**Files:** `spec/uniword/infrastructure/zip_extractor_spec.rb` -30 sites in specs use these to: -1. Force-clear an attribute after construction (to test nil handling) -2. Inspect internal state (when no public reader exists) -3. Inject test doubles into private state +## Status of the original note -## Fix per pattern +Stale. It claimed **30 sites across 12 files**. Actual today: **7 +matches**, **6** of them real, all in one file: -### Pattern 1: Force-clear an attribute -```ruby -# Before -comment = Comment.new(comment_id: "1") -comment.instance_variable_set(:@comment_id, nil) +Before this change: -# After — construct without the attribute -comment = Comment.new +``` +spec/uniword/infrastructure/zip_extractor_spec.rb:37,63,84,136,191,226 +spec/uniword/builder/run_builder_drawing_spec.rb:7 # a test NAME, not a use ``` -### Pattern 2: Inspect internal state -Add a public reader to the model, then assert through it: +After it, the only remaining match in `spec/` is that test name — no real +uses are left. + +The 11 other files listed in the original note were already clean. The +work happened incrementally and nobody updated the note. The three +patterns it describes no longer occur. + +## Problem + +The six remaining sites are all the same line: + ```ruby -# Before -drawings = builder.model.instance_variable_get(:@drawings) - -# After — expose on the model -class Model - def drawings = @drawings -end -drawings = builder.model.drawings +temp_zip.instance_variable_set(:@finalizer, proc {}) +# "Suppress finalizer - we handle cleanup manually via ensure block" ``` -### Pattern 3: Inject test doubles -Refactor to constructor injection: -```ruby -# Before -tf = TempZip.new -tf.instance_variable_set(:@finalizer, proc {}) +**`Tempfile` has no `@finalizer` ivar.** The installed version +(`tempfile-0.3.1`) uses `@finalizer_manager`: -# After — pass via constructor or a public writer -tf = TempZip.new(finalizer: proc {}) ``` +instance ivars: [:@unlinked, :@mode, :@opts, :@delegate_dc_obj, :@finalizer_manager] +after set: [..., :@finalizer_manager, :@finalizer] +``` + +The assignment creates a brand-new ivar that nothing ever reads. It +suppresses nothing. The comment describes behavior the code does not +have. + +The variant inside the `create_temp_zip` helper (line 20 before this +change) is broken differently — it calls `remove_instance_variable` on +the `Tempfile` **class object** rather than on a tempfile instance, then +rescues the resulting `NameError`. Also a no-op. + +So whatever fixed the original Windows `EACCES` flake, it was not this. +The `close` + `safe_delete` calls above it are doing the actual work. + +This inverts the obvious fix. Consolidating the six copies behind one +well-named helper, or documenting a sanctioned exception, would enshrine +dead code and teach the next reader that the project needs an ivar +exception it does not need. + +## Fix + +Single PR. Small. + +What was done: + +1. Deleted all six `instance_variable_set(:@finalizer, proc {})` calls + along with the comments claiming they suppress finalization. +2. Deleted `create_temp_zip` in full — its own `Tempfile.new`, `close` + and `safe_delete` included. It had no callers on either ref, so + nothing lost coverage. +3. Kept `Tempfile.new` and the existing cleanup at all six remaining + sites. That was the minimal correct change; nothing else moved. + +`Tempfile.create` or `Dir.mktmpdir` is **optional and not drop-in**: +`Tempfile.create` returns a plain `File`, which has no `unlink` instance +method, so any existing `temp_zip.unlink` cleanup breaks. A valid +conversion closes and deletes the created file before rubyzip opens the +path, then uses `safe_delete(file.path)` in the `ensure` block. Only do +this if it genuinely simplifies the fixture. + +## Risk + +The `create_temp_zip` comment this change deletes attributed the +workaround to a **Windows** `EACCES` flake, and there is a recent commit +hardening a different Windows save-gate test (`facd8fb`). Development +machines here are macOS, so the flake cannot be reproduced locally +either way. + +The evidence says the ivar cannot be load-bearing — it targets a name +Tempfile does not use. But "cannot possibly matter" is exactly the +reasoning that precedes a surprise, and the surprise here lands on a +platform we cannot test on. + +**Land this alone, on its own branch, and let Windows CI vote.** Do not +bundle it with other work. If CI goes red, the fallback is +`Tempfile.create`, not restoring the no-op. ## Verification -`grep -rn "instance_variable_set\|instance_variable_get" spec/uniword/ | wc -l` -should trend toward 0. All affected specs pass. +- `bundle exec rspec spec/uniword/infrastructure/zip_extractor_spec.rb` + green, run repeatedly to check for a resurfaced flake. +- `grep -rn "instance_variable_set\|instance_variable_get" spec/` returns + only the unrelated test name in `run_builder_drawing_spec.rb`. +- **Windows CI green.** This is the gate that matters; local green proves + little for a Windows-only workaround. +- `bundle exec rubocop spec/uniword/infrastructure/zip_extractor_spec.rb` + +## Out of scope + +- Renaming the `run_builder_drawing_spec.rb` example. Its name + accurately describes what it asserts; it is only a grep false positive. +- `Uniword::Infrastructure::ZipExtractor` itself. Nothing here touches + library code. +- `send`/`__send__` in specs. Different rule, different TODO. diff --git a/spec/uniword/infrastructure/zip_extractor_spec.rb b/spec/uniword/infrastructure/zip_extractor_spec.rb index d6cf45bd..d5a7e8dd 100644 --- a/spec/uniword/infrastructure/zip_extractor_spec.rb +++ b/spec/uniword/infrastructure/zip_extractor_spec.rb @@ -7,23 +7,6 @@ RSpec.describe Uniword::Infrastructure::ZipExtractor do let(:extractor) { described_class.new } - # Helper to create a temporary ZIP file on disk, properly handling Windows file locking. - # On Windows, Tempfile keeps a handle open which conflicts with Zip::File::CREATE's - # atomic rename. This helper closes and deletes the tempfile first, and suppresses - # the Tempfile finalizer to prevent EACCES errors when rubyzip locks files. - def create_temp_zip - temp_zip = Tempfile.new(["test", ".zip"]) - temp_zip.close - safe_delete(temp_zip.path) - # Suppress finalizer - we handle cleanup manually via safe_delete - begin - Tempfile.send(:remove_instance_variable, :@finalizer) - rescue StandardError - nil - end - temp_zip - end - describe "#extract" do context "with valid ZIP file" do it "extracts all files from ZIP archive" do @@ -33,8 +16,6 @@ def create_temp_zip temp_zip = Tempfile.new(["test", ".zip"]) temp_zip.close safe_delete(temp_zip.path) - # Suppress finalizer - we handle cleanup manually via ensure block - temp_zip.instance_variable_set(:@finalizer, proc {}) begin Zip::File.open(temp_zip.path, Zip::File::CREATE) do |zip_file| zip_file.get_output_stream("file1.txt") { |f| f.write("Content 1") } @@ -59,8 +40,6 @@ def create_temp_zip temp_zip = Tempfile.new(["test", ".zip"]) temp_zip.close safe_delete(temp_zip.path) - # Suppress finalizer - we handle cleanup manually via ensure block - temp_zip.instance_variable_set(:@finalizer, proc {}) begin Zip::File.open(temp_zip.path, Zip::File::CREATE) do |zip_file| zip_file.mkdir("empty_dir") @@ -80,8 +59,6 @@ def create_temp_zip temp_zip = Tempfile.new(["test", ".zip"]) temp_zip.close safe_delete(temp_zip.path) - # Suppress finalizer - we handle cleanup manually via ensure block - temp_zip.instance_variable_set(:@finalizer, proc {}) begin Zip::File.open(temp_zip.path, Zip::File::CREATE) { |_zip_file| } @@ -132,8 +109,6 @@ def create_temp_zip tf = Tempfile.new(["test", ".zip"]) tf.close safe_delete(tf.path) - # Suppress finalizer - we handle cleanup manually via after block - tf.instance_variable_set(:@finalizer, proc {}) tf end @@ -187,8 +162,6 @@ def create_temp_zip tf = Tempfile.new(["test", ".zip"]) tf.close safe_delete(tf.path) - # Suppress finalizer - we handle cleanup manually via after block - tf.instance_variable_set(:@finalizer, proc {}) tf end @@ -222,8 +195,6 @@ def create_temp_zip temp_empty = Tempfile.new(["empty", ".zip"]) temp_empty.close safe_delete(temp_empty.path) - # Suppress finalizer - we handle cleanup manually via ensure block - temp_empty.instance_variable_set(:@finalizer, proc {}) begin Zip::File.open(temp_empty.path, Zip::File::CREATE) { |_zip_file| }