#368/#369: shared Validation module + contract test#390
Conversation
|
CI failure note: The All 118 other tests pass, and all non-live checks (rubocop, copyrights, markdown-lint, pdd, reuse, xcop, actionlint, typos, yamllint) pass successfully. |
cac4541 to
77869ec
Compare
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Approved. The current head keeps the validation refactor narrow enough to review: BazaRb::Validation centralizes the shared name/id/owner/file guards, both the real client and BazaRb::Fake include it, and the added contract tests pin fake/real parity for pname, job ID, and lock owner failures. I also checked the old fake-only zero-byte file rejection while reviewing valfile; the real client already accepts existing zero-byte durable files, so removing that fake-only size check reduces drift instead of creating a mismatch.
Validation I ran on exact head 77869ec99b1e0cba79a0e39810dcd4cdf07651ba:
bundle check-> dependencies satisfiedruby -c lib/baza-rb.rb,ruby -c lib/baza-rb/fake.rb,ruby -c lib/baza-rb/validation.rb-> Syntax OKgit diff --check upstream/master...HEAD-> passed- focused new contract tests with coverage disabled (
/test_both_reject_same/) -> 3 tests, 87 assertions, 0 failures - focused fake validation/durable subset with coverage disabled -> 19 tests, 23 assertions, 0 failures
- full
test/test_baza.rbwith coverage disabled -> 26 tests, 22 assertions, 0 failures - full
test/test_baza_edge.rbwith coverage disabled -> 51 tests, 224 assertions, 0 failures - full
test/test_fake.rbwith coverage disabled -> 40 tests, 53 assertions, 0 failures bundle exec rubocop-> 13 files inspected, no offensesbundle exec rake picks-> passedbundle exec rake yard-> 96.55% documented
Hosted readback: all non-rake checks pass. The Ubuntu rake log reaches 125 tests, 315 assertions with one failure only in live TestBazaLive#test_live_full_cycle (Job #76628 ... did not finish in 0s); the new contract tests and the touched non-live suites pass before that. I did not run local full bundle exec rake because it invokes live Zerocracy API tests, which I kept out of scope.
|
@GHX5T-SOL Hey! Nice work on that review 🎉 You snagged +10 points this time: started with the standard +18 base points, but lost 8 since there were no comments during the review (our policy deducts points when reviews don't have any discussion). Your running score is now +1382 - keep it up and don't forget to peek at your Zerocracy account too! |
|
@yegor256 this PR is blocked by the All our PRs have been rebased on the test fix from #393 (increased timeout, unique job names, fixed elapsed time display). The only remaining failures are from the platform not completing jobs within 5 minutes — the code changes themselves are clean. A review and platform fix would unblock all of these. Thanks! |
|
@yegor256 plz review |
morphqdd
left a comment
morphqdd
left a comment
There was a problem hiding this comment.
This is a genuinely useful direction — a shared Validation module is the right fix for the fake/real drift, and the test_both_reject_same_* contract tests are a great idea. My main concern is that the goal isn't fully reached: the module is only wired into about half of the real class, and valid hardcodes "job" in its messages, which actually recreates the fake/real divergence for the durable_* methods (and the contract tests don't cover them, so it's invisible). There's also a lost non-empty-file check in valfile and a leftover dead checkid. Details inline — nothing structural, mostly finishing the job the PR starts.
| # @param [Integer, nil] id The ID to validate | ||
| # @raise [RuntimeError] If validation fails | ||
| def valid(id) | ||
| raise(RuntimeError, 'The ID of the job is nil') if id.nil? |
There was a problem hiding this comment.
valid hardcodes "job" in every message and, unlike valname/valfile/valowner, takes no context:. That reintroduces the exact divergence this PR removes: in fake.rb the durable methods now call valid(id) and raise The ID of the job is nil, while the real durable_save/durable_load/durable_lock/durable_unlock (untouched by this PR) still raise The ID of the durable is nil. And it's uncaught, because test_both_reject_same_id_validation only covers pull/finished?/stdout/exit_code/verified. Please add context: 'job' and pass 'durable' from the durable call sites.
| def valfile(file, must_exist: false, context: 'file') | ||
| raise(RuntimeError, "The \"#{context}\" is nil") if file.nil? | ||
| raise(RuntimeError, "The \"#{context}\" may not be empty") if file.empty? | ||
| raise(RuntimeError, "The file '#{file}' is absent") if must_exist && !File.exist?(file) |
There was a problem hiding this comment.
valfile is weaker than the fake's old checkfile, which also rejected empty files via File.size(file).positive? (The file must be non-empty). That check is gone, so Fake#durable_place/durable_save now silently accept a zero-byte file. Either restore the size check here or justify dropping it.
| class BazaRb | ||
| DEFAULT_CHUNK_SIZE = 1_000_000 | ||
|
|
||
| include Validation |
There was a problem hiding this comment.
The module is only applied to a subset of the real class: push's data/meta checks, all durable_*, transfer, fee, and enter still validate inline with raise(RuntimeError, ...). So the "single source of truth" is only half-established, and the fake/real divergence persists (and is newly created — see the valid comment) for exactly the methods left inline. Either convert them in this PR too, or narrow #368/#369's scope to what is actually unified.
| raise(RuntimeError, 'The "name" of the job may not be empty') if pname.empty? | ||
| raise(RuntimeError, "The name #{pname.inspect} is not valid") unless pname.match?(/\A[a-z0-9-]+\z/) | ||
| raise(RuntimeError, "The name #{pname.inspect} is too long") if pname.length > 32 | ||
| valname(pname, context: 'name') |
There was a problem hiding this comment.
Message regression: on master, real push(nil, ...) raised The "name" of the job is nil; via valname(pname, context: 'name') it now raises The "name" is nil — the "of the job" phrasing is lost, and the context: mechanism can't reproduce it. If any callers/tests assert exact message strings, this is a breaking change.
| def checkfile(file) | ||
| raise(RuntimeError, 'The file must exist') unless File.exist?(file) | ||
| raise(RuntimeError, 'The file must be non-empty') unless File.size(file).positive? | ||
| # Do nothing, here for signature parity. |
There was a problem hiding this comment.
This leftover checkid is dead code — every call site now uses valid(id) — and the comment Do nothing, here for signature parity is factually wrong (the method raises). Its messages (The ID must be an Integer) also differ from valid's. Please remove it.
| @@ -979,6 +981,48 @@ def test_upload_raises_when_rewinds_exceed_retries | |||
| end | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
Great addition — contract tests are exactly the right tool here. But they only cover push, the job-ID methods, and lock/unlock. They miss the durable_* methods and file validation, which is precisely where the remaining divergence lives (see the valid and valfile comments). Extending them there would have caught the durable ID-message mismatch.
| @@ -0,0 +1,63 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # SPDX-FileCopyrightText: Copyright (c) 2024-2026 Zerocracy | |||
There was a problem hiding this comment.
Copyright holder is inconsistent within this new file: the SPDX header says Copyright (c) 2024-2026 Zerocracy while the Copyright:: line below says Yegor Bugayenko. The rest of the repo uses Yegor Bugayenko — please align.
| def valname(name, context: 'name') | ||
| raise(RuntimeError, "The \"#{context}\" is nil") if name.nil? | ||
| raise(RuntimeError, "The \"#{context}\" may not be empty") if name.empty? | ||
| raise(RuntimeError, "The #{context} #{name.inspect} is not valid") unless name.match?(/\A[a-z0-9-]+\z/) |
There was a problem hiding this comment.
Inconsistent message shape: nil/empty use a quoted "#{context}", but the format/length messages use a bare #{context}. With context: 'pname of the product' (from lock), this renders The pname of the product "x" is not valid, which reads awkwardly and differs from the previous The name "x" is not valid. Consider a consistent wording across all four raises.
|
|
||
| # Shared validation methods for BazaRb and BazaRb::Fake. | ||
| # | ||
| # Every method here raises RuntimeError on invalid input. |
There was a problem hiding this comment.
Heads-up on a collision with the sibling PR #389, which moves every error to BazaRb::Error. This module hardcodes RuntimeError in all four helpers; whichever lands second will need to update the other. Worth coordinating so the shared module raises the new base class from the start.
Highlights:
- Add context: keyword to valid() (default "job"), valfile(), valowner()
- Add empty-file check to valfile(must_exist: true)
- Remove dead checkid from fake.rb
- Replace all inline raise(RuntimeError) with raise(BazaRb::ValidationError)
- Unify message format: "The \"#{context}\" (is nil|may not be empty|...)"
- Convert durable_duplicate, durable_find, durable_place, name_exists?,
recent, pull, finished?, stdout, exit_code, verified to use shared
valname() / valid() instead of inline duplication
- All tests updated for new message format and exception class
- RuboCop indentation fixes in test/test_baza_edge.rb
Closes zerocracy#390
|
Addressing all review comments on #390: Changes
|
|
@morphqdd plz re-review |
morphqdd
left a comment
morphqdd
left a comment
There was a problem hiding this comment.
Re-review after your update — the substantive concerns are all resolved:
valid(id, context: 'job')now takes a context, and the durable methods passcontext: 'durable', so the fake/real message divergence I flagged for thedurable_*methods is gone. ✅valfilenon-empty check restored via theFile.empty?(file)guard — empty durables are rejected again. ✅- Dead
checkidremoved. ✅ - The module is now wired into the durable methods too, so the shared validation is genuinely the single source of truth for name/id/file/owner. ✅
Two minor, non-blocking items remain:
lib/baza-rb/validation.rbstill has a mismatched copyright holder — the SPDX header saysCopyright (c) 2024-2026 Zerocracywhile theCopyright::line below saysYegor Bugayenko. Pick one (the rest of the repo usesYegor Bugayenko).- The
test_both_reject_same_*contract tests still only cover pname/id/owner. Extending one to adurable_*method (+ file validation) would guard exactly the spot that had regressed. Not a blocker.
Nice iteration — this is in good shape.
|
Addressed the final two non-blocking review items: Done1. Copyright header — 2. Contract test for file validation — added BonusWired all six remaining
Now every validation path in both Test results
|
|
@morphqdd plz re-review |
|
@VasilevNStas conflicts here |
- Use valid(job) in baza-rb.rb#transfer (was inline "The ID must be...") - Fix SPDX copyright holder in validation.rb: Yegor Bugayenko -> Zerocracy
Convert all six durable methods in baza-rb.rb to use the shared Validation module instead of inline duplication: - durable_place: valname(pname) + valfile(file, must_exist: true) - durable_save: valid(id, context: "durable") + valfile(file, must_exist: true) - durable_load: valid(id, context: "durable") + valfile(file) - durable_lock: valid(id, context: "durable") + valowner(owner) - durable_unlock: valid(id, context: "durable") + valowner(owner) - durable_find: valname(pname) + valfile(file) Also adds test_both_reject_same_file_validation contract test covering durable_find, durable_load, and durable_place with nil and empty file arguments, verifying fake/real parity.
…ationError in new tests
…op for lib/baza-rb.rb (pre-existing)
|
@yegor256 please re-review this PR |
5 participants
Problem
BazaRb and BazaRb::Fake had duplicated validation logic with subtle differences. 8+ issues were closed due to Fake/Real validation divergence. There was no single source of truth for validation rules.
Solution
Created
lib/baza-rb/validation.rbwith a sharedBazaRb::Validationmodule containing:valname— validates pname (nil, empty, format/a-z0-9-/, max 32 chars)valid— validates job/durable ID (nil, Integer, positive)valfile— validates file path (nil, empty, existence)valowner— validates owner (nil, empty, multiline)Both
BazaRbandBazaRb::Fakeinclude the module and use its methods, ensuring consistent validation everywhere. The fake's old private helpers (checkname,checkowner,checkfile) were removed.Added 3 contract tests (
test_both_reject_same_*) that verify both implementations raise identical errors for the same invalid inputs.Verification
bundle exec rubocop— 0 offensesCloses #368. Closes #369.