Skip to content

#400: fee() missing positive integer validation for job parameter#409

Merged
yegor256 merged 2 commits into
zerocracy:masterfrom
VasilevNStas:400-fee-positive
Jul 13, 2026
Merged

#400: fee() missing positive integer validation for job parameter#409
yegor256 merged 2 commits into
zerocracy:masterfrom
VasilevNStas:400-fee-positive

Conversation

@VasilevNStas

Copy link
Copy Markdown
Contributor

Fixes #400.

fee() checked job for nil and Integer type but did not validate that it is positive, unlike transfer() which has unless job.positive?.

  • Adds raise(RuntimeError, 'The "job" must be positive') unless job.positive? to fee in lib/baza-rb.rb
  • Same fix in lib/baza-rb/fake.rb
  • Adds test test_fee_raises_when_job_is_not_positive to both test_baza_edge.rb and test_fake.rb

@morphqdd morphqdd 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.

Correct and complete — the job.positive? guard now mirrors transfer, applied symmetrically to the real client, the fake, and both test files with exact-message assertions. LGTM.

One related observation inline (not part of this PR's scope).

Comment thread lib/baza-rb.rb
raise(RuntimeError, 'The "amount" must be positive') unless amount.positive?
raise(RuntimeError, 'The "job" is nil') if job.nil?
raise(RuntimeError, 'The "job" must be Integer') unless job.is_a?(Integer)
raise(RuntimeError, 'The "job" must be positive') unless job.positive?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same 'mirror transfer' rationale, for a follow-up: transfer also rejects an empty summary (summary.empty?), whereas fee (just below) still only checks summary.nil?, so fee('x', 1.0, '', 42) passes an empty summary to the server. That specific gap is handled in #406 — flagging so the two land consistently.

@0crat

0crat commented Jul 7, 2026

Copy link
Copy Markdown

@morphqdd Thanks for the review! You've earned +9 points for this: +18 as a basis; -5 for very few (1) comments; -4 for too few (16) hits-of-code. Your running score is +632; don't forget to check your Zerocracy account too).

@yegor256

Copy link
Copy Markdown
Member

@VasilevNStas merge conflict here

@VasilevNStas

Copy link
Copy Markdown
Contributor Author

@yegor256

This issue is still actively blocking all PRs from merging. Here is a detailed breakdown of why CI now takes 30+ minutes and what can be done about it.

Current state

The CI matrix was expanded from 2 jobs to 4 jobs (macOS-15 + Ubuntu-24.04 × Ruby 3.3 + 3.4). Each job runs the full suite including test_live_full_cycle.

What happened on the latest run (PR #409, run #29231732468)

Job Started Ended Duration Status
ubuntu-24.04, 3.3 07:20 ~33min cancelled
macos-15, 3.3 07:28 ~25min cancelled
macos-15, 3.4 07:37 ~16min cancelled
ubuntu-24.04, 3.4 queued (never started)

Total wall-clock time: 33 minutes, then manually cancelled.

Why it is so slow

  1. test_live_full_cycle (the root cause) — this test submits a job to api.zerocracy.com and polls .finished? for up to 15 minutes. When the platform is slow or the job gets stuck, every single job in the matrix waits the full 15 minutes before the test fails. This is the single biggest contributor to CI latency.

  2. Matrix expansion (amplifier) — with 4 jobs instead of 2, the probability that at least one job hits the slow platform window increases. And when a job is waiting on test_live_full_cycle, it holds a GitHub Actions runner hostage for 15 minutes doing nothing but sleep(5) + poll.

  3. Runner queue time — macOS runners on GitHub Actions have limited availability. In this run, the macOS jobs started 8–17 minutes after the Ubuntu job, adding queue latency on top of the test latency.

Impact

  • Every PR waits 15–35 minutes for CI to complete (or timeout)
  • Developers cannot get quick feedback on their changes
  • Multiple PRs are blocked waiting for green CI
  • The test_live_full_cycle failure is never caused by the PR's code — it is always a platform issue

Suggested solutions

Option A: Move live tests out of rake default

Create a separate rake live task and remove test_live_* from the default suite. Live tests would still run nightly or on demand, but they would not block PR merges. This is the lowest-risk change.

Option B: Reduce timeout and mark as flaky

Reduce wait_for in test_live_full_cycle back to 2–3 minutes. The test will sometimes fail when the platform is slow, but CI will complete in 3–5 minutes instead of 15–35. A flaky test is better than a permanently slow CI.

Option C: Skip live tests in CI via environment variable

Add a guard so that test_live_* are skipped when a CI=true or TEST_LIVE=false environment variable is set. CI gets the full test suite minus the platform-dependent tests; developers can still run TEST_LIVE=true locally.

Option D: Server-side fix (Zerocracy platform)

Ensure test jobs complete within 2 minutes, or provide a dedicated fast-path endpoint for CI. This is the ideal fix but requires changes outside this repository.

I would be happy to implement any of options A, B, or C — just let me know which approach you prefer.

@yegor256 yegor256 merged commit 0653fb6 into zerocracy:master Jul 13, 2026
8 of 12 checks passed
@0crat

0crat commented Jul 14, 2026

Copy link
Copy Markdown

@VasilevNStas Thanks for the contribution! You've earned +20 points for this: +24 as a basis; -4 for too few (16) hits-of-code. Please, keep them coming. Your running score is +2776; don't forget to check your Zerocracy account too).

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.

fee() method missing positive integer validation for job parameter

4 participants