Skip to content

Increase waiting time liquid tests#256

Merged
michieldegezelle merged 2 commits into
mainfrom
increase_waiting_time_liquid_tests
Jun 8, 2026
Merged

Increase waiting time liquid tests#256
michieldegezelle merged 2 commits into
mainfrom
increase_waiting_time_liquid_tests

Conversation

@michieldegezelle

Copy link
Copy Markdown
Contributor

Fixes # (link to the corresponding issue if applicable)

Description

Some large PR's keep having failed liquid test runs because a lot of files were touched. If the tests run > 16 mins, the test run crashes, making merging very hard.

Testing Instructions

Steps:

  1. ...
  2. ...
  3. ...

Author Checklist

  • Skip bumping the CLI version

Reviewer Checklist

  • PR has a clear title and description
  • Manually tested the changes that the PR introduces
  • Changes introduced are covered by tests of acceptable quality

@michieldegezelle michieldegezelle self-assigned this Jun 8, 2026
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This patch release extends the polling timeout in the test runner to reduce timeout errors during test execution. The code change doubles the waiting limit threshold, accompanied by a version bump to 1.56.1 and corresponding changelog documentation.

Changes

Test Run Timeout Extension (v1.56.1 patch)

Layer / File(s) Summary
Polling timeout limit increase
lib/liquidTestRunner.js
In fetchResult, the polling timeout waitingLimit was increased from 1000000 to 2000000 milliseconds to prevent test run timeouts.
Version bump and changelog
package.json, CHANGELOG.md
Package version updated to 1.56.1 and changelog entry added documenting the timeout extension.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • silverfin/silverfin-cli#252: Previous PR that modified the same polling timeout in lib/liquidTestRunner.js with similar version and changelog updates for timeout error prevention.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is largely incomplete with placeholder testing instructions and all reviewer checklist items unchecked, making it inadequate for review. Replace placeholder testing steps with actual instructions, provide the issue link if applicable, and complete all checklist items to indicate readiness.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely summarizes the main change: increasing the waiting time for liquid tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch increase_waiting_time_liquid_tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
lib/liquidTestRunner.js (1)

311-334: ⚡ Quick win

Consider adding progress logging for long-running tests.

With the timeout now at 33+ minutes, users may wait a long time without feedback. Consider adding periodic log messages (e.g., "Still running tests... (15 minutes elapsed)") to reassure users that the process hasn't stalled.

💡 Suggested enhancement
 async function fetchResult(firmId, testRunId, templateType) {
   let testRun = { status: "started" };
   let pollingDelay = 1000;
   const waitingLimit = 2000000;
+  const logIntervals = [600000, 1200000, 1800000]; // 10, 20, 30 minutes
+  let nextLogIndex = 0;

   spinner.spin("Running tests..");
   let waitingTime = 0;
   while (testRun.status === "started") {
     await new Promise((resolve) => {
       setTimeout(resolve, pollingDelay);
     });
     const response = await SF.readTestRun(firmId, testRunId, templateType);
     testRun = response.data;
     waitingTime += pollingDelay;
     pollingDelay *= 1.05;
+    
+    if (nextLogIndex < logIntervals.length && waitingTime >= logIntervals[nextLogIndex]) {
+      spinner.stop();
+      consola.info(`Still running tests... (${Math.floor(waitingTime / 60000)} minutes elapsed)`);
+      spinner.spin("Running tests..");
+      nextLogIndex++;
+    }
+    
     if (waitingTime >= waitingLimit) {
       spinner.stop();
       consola.error("Timeout. Try to run your test again");
       break;
     }
   }
   spinner.stop();
   return testRun;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/liquidTestRunner.js` around lines 311 - 334, The fetchResult loop can
leave users waiting without feedback; add periodic progress logs by introducing
a lastLogged timestamp and a logInterval (e.g., 5 minutes) inside the while
(testRun.status === "started") loop, and when waitingTime - lastLogged >=
logInterval call consola.info with a message like "Still running tests... (X
minutes elapsed)" (compute X from waitingTime), then update lastLogged; keep
spinner behavior unchanged and ensure variables referenced are fetchResult,
waitingTime, pollingDelay, lastLogged, and logInterval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@lib/liquidTestRunner.js`:
- Around line 311-334: The fetchResult loop can leave users waiting without
feedback; add periodic progress logs by introducing a lastLogged timestamp and a
logInterval (e.g., 5 minutes) inside the while (testRun.status === "started")
loop, and when waitingTime - lastLogged >= logInterval call consola.info with a
message like "Still running tests... (X minutes elapsed)" (compute X from
waitingTime), then update lastLogged; keep spinner behavior unchanged and ensure
variables referenced are fetchResult, waitingTime, pollingDelay, lastLogged, and
logInterval.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6e6e667a-6aca-454f-9a91-6017171959e2

📥 Commits

Reviewing files that changed from the base of the PR and between a66cebf and d3b4c98.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • CHANGELOG.md
  • lib/liquidTestRunner.js
  • package.json

@michieldegezelle michieldegezelle merged commit 9cdff2d into main Jun 8, 2026
3 checks passed
@michieldegezelle michieldegezelle deleted the increase_waiting_time_liquid_tests branch June 8, 2026 13:00
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