Skip to content

Conversation

@chetanbothra
Copy link
Contributor

@chetanbothra chetanbothra commented May 28, 2025

This PR introduces automated E2E test workflows to improve testing consistency and catch issues early across different environments.

Workflow Summary:

Nightly Runs: npm run test-mainnet-viem-combined runs every night automatically.

On Commit to master: test-mainnet-viem-combined.

On Commit to develop: npm run test-testnet-viem-combined.

Note: We currently do not have a develop branch

Alerts: All test alerts are sent to the #e2e channel on Slack for immediate visibility.

Summary by CodeRabbit

  • New Features

    • Added top-level CI parameters and a dedicated "run E2E only" workflow to run end-to-end tests independently.
  • Chores

    • Split E2E into branch-specific testnet vs mainnet jobs; publishing now requires successful mainnet E2E.
    • Parameterized E2E commands with sensible defaults and per-job overrides.
    • Centralized test artifacts into a reports location and simplified Slack reporting.
    • Expanded E2E environment setup to populate additional keys and wallet addresses.

@linear
Copy link

linear bot commented May 28, 2025

@coderabbitai
Copy link

coderabbitai bot commented May 28, 2025

Warning

Rate limit exceeded

@chetanbothra has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 34 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 188c766 and 1f81caa.

📒 Files selected for processing (1)
  • .circleci/config.yml (3 hunks)

Walkthrough

Adds a top-level test_command parameter and enables run_e2e_tests; parameterizes the e2e-tests job; adds git checkout master in the e2e flow; introduces run-e2e-only workflow and splits E2E into branch-specific testnet-e2e-tests and mainnet-e2e-tests; replaces mochawesome artifact handling and moves artifacts to /home/circleci/e2e-sdk-modular/reports/test-report; expands environment substitutions for multiple keys and addresses; updates Slack/reporting references.

Changes

Cohort / File(s) Change Summary
CircleCI top-level config
/.circleci/config.yml
Added pipeline parameters: test_command (string, default "npm run test-all-chains") and run_e2e_tests (boolean, default true).
E2E job parameters & steps
/.circleci/config.yml
Added job parameter test_command (string, default "npm run test-testnet-viem-combined"); e2e steps now execute << parameters.test_command >>; added git checkout master in the e2e flow; expanded sed/env substitutions to include primary_private_key, secondary_private_key, primary_wallet_address, secondary_wallet_address, paymaster_address, sponsor_address; replaced mochawesome-specific artifact handling and now stores artifacts under /home/circleci/e2e-sdk-modular/reports with destination test-report.
Workflows & wiring
/.circleci/config.yml
Introduced run-e2e-only workflow (runs when run_e2e_tests is true) that invokes e2e-tests with test_command="npm run test-all-chains"; modified install-and-publish to skip when E2E-only runs and split E2E into testnet-e2e-tests (develop, npm run test-testnet-viem-combined) and mainnet-e2e-tests (master, npm run test-mainnet-viem-combined).
Publish & reporting
/.circleci/config.yml
publish-npm-package now requires mainnet-e2e-tests (instead of generic e2e-tests); publish-github-release branch filter preserved; Slack/report notifications updated to reference /home/circleci/e2e-sdk-modular/reports/test-report and the new artifact layout; removed mochawesome-specific references.

Sequence Diagram(s)

sequenceDiagram
    participant Pipeline
    participant Workflows
    participant E2E as "E2E Job"
    participant Publish as "Publish NPM"

    Note over Pipeline: params `run_e2e_tests`, `test_command`

    alt run_e2e_tests == true
        Pipeline->>Workflows: run-e2e-only
        Workflows->>E2E: e2e-tests(test_command="npm run test-all-chains")
        E2E->>E2E: git checkout master\nrun << parameters.test_command >>
        E2E-->>Workflows: result + artifacts (/home/circleci/e2e-sdk-modular/reports/test-report)
    else run_e2e_tests == false
        Pipeline->>Workflows: install-and-publish or branch flows
        alt branch == develop
            Workflows->>E2E: testnet-e2e-tests(test_command="npm run test-testnet-viem-combined")
            E2E->>E2E: run << parameters.test_command >>
            E2E-->>Workflows: result + artifacts (/home/circleci/e2e-sdk-modular/reports/test-report)
        else branch == master
            Workflows->>E2E: mainnet-e2e-tests(test_command="npm run test-mainnet-viem-combined")
            E2E->>E2E: git checkout master\nrun << parameters.test_command >>
            E2E-->>Publish: success -> publish-npm-package dependency satisfied
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I hop through YAML, nudging commands to start,
Branches split like paths across the mart.
Reports now nest in /home/circleci/.../reports/test-report,
Keys and addresses tucked in every part.
Master winks — the publish routine can start. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description does not follow the repository’s required template as it lacks the "## Description" heading, omits the "Types of changes" checklist section, and does not include any template checkboxes or structured sections, making it incomplete against template requirements. Please update the description to include the "## Description" section detailing the changes, add the "Types of changes" checklist with appropriate checkboxes, and fill in any other required template sections such as further comments if needed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title is concise and clearly summarizes the main change in the pull request, highlighting enhancements to E2E test automation workflows without extraneous details, making it easily identifiable in the project history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

@chetanbothra chetanbothra requested review from boonet and ch4r10t33r May 28, 2025 16:09
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
.circleci/config.yml (1)

271-271: Remove extraneous blank line
There’s an extra blank line at the end of the file, triggering a YAML lint warning. Removing it will satisfy the empty-lines rule.

🧰 Tools
🪛 YAMLlint (1.37.1)

[warning] 271-271: too many blank lines

(1 > 0) (empty-lines)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a18a627 and 875dbd4.

📒 Files selected for processing (1)
  • .circleci/config.yml (4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.circleci/config.yml

[warning] 271-271: too many blank lines

(1 > 0) (empty-lines)

🔇 Additional comments (7)
.circleci/config.yml (7)

50-54: Job-level test_command parameter looks good
The e2e-tests job now cleanly accepts a test_command parameter (lines 50–54), enabling workflows to override the test command dynamically.


104-104: Verify parameter substitution within a multiline script
You’ve injected << parameters.test_command >> inside a bash command: | block. Please confirm that CircleCI expands parameters inside literal blocks as intended so the correct test command runs.


233-235: install-and-publish conditional is correct
Using when: not << pipeline.parameters.run_e2e_tests >> properly gates this workflow when E2E tests aren’t explicitly requested.


240-246: Validate branch filter for testnet E2E tests
The testnet-e2e-tests job is filtered to the develop branch (lines 240–246), but a develop branch doesn’t currently exist. Please confirm or update this filter to match your branch strategy.


247-254: Confirm default branch for mainnet E2E tests
The mainnet-e2e-tests job filters on master (lines 247–254). If your default branch is named main, this job won’t trigger—please verify and adjust accordingly.


258-262: Align publish-npm-package branch filter with default branch
Publishing is gated to master (lines 258–262). Ensure this matches your repository’s primary branch to avoid missing npm releases.


270-271: Check GitHub release workflow branch filter
The publish-github-release job is also restricted to master (lines 270–271); verify that this aligns with your actual default branch for consistent release automation.

🧰 Tools
🪛 YAMLlint (1.37.1)

[warning] 271-271: too many blank lines

(1 > 0) (empty-lines)

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

@chetanbothra chetanbothra requested a review from kanthgithub May 29, 2025 09:14
@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.circleci/config.yml (1)

114-126: Critical: Test report path mismatch will break Slack notifications.

Lines 114-116 copy the test report to /tmp/test-report/, but line 125 attempts to download from /tmp/mochawesome-report/mochawesome.json. This path inconsistency will cause the Slack notification step to fail when trying to parse test results.

Apply this fix to align the paths:

-                mkdir -p /tmp/test-report
-                cp -r reports/test-report.html /tmp/test-report/
-                chmod 777 -R /tmp/test-report
+                mkdir -p /tmp/mochawesome-report
+                cp -r reports/* /tmp/mochawesome-report/
+                chmod 777 -R /tmp/mochawesome-report

Or alternatively, if you want to keep /tmp/test-report, update line 125:

-                wget https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/mochawesome-report/mochawesome.json
+                wget https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/mochawesome.json

Also update lines 161 and 171 to reference the correct HTML report path.

♻️ Duplicate comments (1)
.circleci/config.yml (1)

6-8: Pipeline parameter test_command is declared but never consumed.

The pipeline-level parameter test_command is defined but all workflows pass literal string values to the job (e.g., lines 238, 249, 257) instead of referencing << pipeline.parameters.test_command >>. This makes the parameter declaration redundant.

Either remove this parameter declaration or utilize it in your workflows if you intend to override test commands via API calls.

🧹 Nitpick comments (1)
.circleci/config.yml (1)

244-245: Orphaned job: install runs but nothing depends on it.

The install job executes in the install-and-publish workflow but no subsequent jobs declare it as a requirement. Both E2E jobs perform their own installation steps independently (lines 66-82), making this job redundant and wasteful of CI resources.

Either remove the install job from this workflow or make the E2E jobs depend on it (add requires: [install]) and leverage its cache.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2044237 and bf25be3.

📒 Files selected for processing (1)
  • .circleci/config.yml (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (4)
.circleci/config.yml (4)

50-53: LGTM! Job parameter properly declared.

The test_command parameter is correctly defined and consumed at line 110, enabling flexible test execution across workflows.


247-253: Verify branch existence: testnet-e2e-tests filters for non-existent develop branch.

The PR objectives note that "a develop branch does not currently exist." This job will never trigger until the branch is created, which may be intentional preparation but could also indicate a misconfiguration.

Confirm whether this is intentional future-proofing or if the branch filter should target an existing branch.


99-105: Verify all required environment variables are configured in CircleCI context.

These new environment variable substitutions expand the E2E test setup with additional private keys, wallet addresses, and contract addresses. Ensure all of these variables are properly set in the general-vars context:

  • API_KEY_SESSIONKEY
  • PRIMARY_PRIVATE_KEY, SECONDARY_PRIVATE_KEY
  • PRIMARY_WALLET_ADDRESS, SECONDARY_WALLET_ADDRESS
  • PAYMASTER_ADDRESS, SPONSOR_ADDRESS

Missing variables will cause silent substitution failures where placeholders remain in the .env file.


262-277: LGTM! Publish jobs correctly filtered to master branch.

The branch filters ensure that npm package publication and GitHub releases only occur on the master branch after mainnet E2E tests pass. This aligns with the PR objectives.

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
.circleci/config.yml (1)

6-8: Remove or utilize the unused top-level test_command pipeline parameter.

The pipeline-level parameter is declared but never consumed. At line 238, the run-e2e-only workflow passes a hardcoded string instead of << pipeline.parameters.test_command >>. Either remove these lines or leverage the parameter to avoid redundancy.

Based on past review comments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf25be3 and d70b10c.

📒 Files selected for processing (1)
  • .circleci/config.yml (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (4)
.circleci/config.yml (4)

50-53: LGTM!

The job-level parameter allows flexible test command configuration and is correctly consumed at line 110.


99-105: LGTM!

The expanded environment variable substitutions provide additional configuration flexibility for E2E tests and follow the established pattern.


233-238: LGTM!

The conditional workflow correctly enables standalone E2E test execution when triggered via pipeline parameter.


262-269: LGTM!

The dependency update correctly gates npm publishing on mainnet E2E test success, aligning with the split job architecture.

Comment on lines 114 to 119
mkdir -p /tmp/test-report
cp -r reports/test-report.html /tmp/test-report/
chmod 777 -R /tmp/test-report
when: always
- store_artifacts:
path: /tmp/mochawesome-report
path: /tmp/test-report
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix artifact path inconsistency.

The artifact path was updated to /tmp/test-report, but line 125 still attempts to download from the old path /tmp/mochawesome-report/mochawesome.json. This will cause the test result parsing and Slack notification to fail.

Apply this diff to fix the wget command:

-                wget https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/mochawesome-report/mochawesome.json
+                wget https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/mochawesome.json

Also update the HTML report URLs in the Slack messages at lines 161 and 171:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/mochawesome-report/mochawesome.html|View HTML Report>\n\
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>\n\
🤖 Prompt for AI Agents
In .circleci/config.yml around lines 114-119 (and update lines 125, 161 and
171), the artifact directory was changed to /tmp/test-report but downstream
commands still reference the old /tmp/mochawesome-report path; update the
wget/download command at ~line 125 to fetch mochawesome.json from
/tmp/test-report instead of /tmp/mochawesome-report, and update the HTML report
URLs used in the Slack notification blocks at ~lines 161 and 171 to point to the
new /tmp/test-report HTML file(s); ensure all references (wget, file paths in
notifications) consistently use /tmp/test-report.

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
.circleci/config.yml (2)

6-8: Remove unused pipeline parameter.

The pipeline-level test_command parameter is declared but never consumed. Line 255 hardcodes the value instead of referencing << pipeline.parameters.test_command >>. Either remove this parameter declaration or use it in the workflows.


257-270: testnet-e2e-tests filtered to non-existent develop branch.

The job is configured to run only on the develop branch, which the PR objectives confirm does not currently exist. This means testnet E2E tests will never execute until the branch is created.

If this is intentional future-proofing, document it. Otherwise, update the filter to an existing branch (e.g., a feature branch pattern) or create the develop branch.

🧹 Nitpick comments (1)
.circleci/config.yml (1)

138-199: Consider removing or updating commented notification code.

This large block of commented code references the old artifact paths and mochawesome format. If this detailed notification will be re-enabled later, update the paths to match the new structure. Otherwise, consider removing it to reduce clutter.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da1180f and 6f82f83.

📒 Files selected for processing (1)
  • .circleci/config.yml (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (6)
.circleci/config.yml (6)

50-53: LGTM! Job parameterization implemented correctly.

The test_command job parameter enables flexible test execution across different workflows and branches.


99-105: Verify that new environment variables are defined in CircleCI context.

The configuration references several new environment variables. Ensure API_KEY_SESSIONKEY, PRIMARY_PRIVATE_KEY, SECONDARY_PRIVATE_KEY, PRIMARY_WALLET_ADDRESS, SECONDARY_WALLET_ADDRESS, PAYMASTER_ADDRESS, and SPONSOR_ADDRESS are defined in the general-vars context in CircleCI.


110-110: LGTM! Test command properly parameterized.

The test execution now uses the job parameter, enabling different test suites for different branches.


114-120: LGTM! Test report artifact path updated consistently.

The artifact path has been correctly updated to /tmp/test-report and the corresponding file copy operation references the correct source location.


250-256: LGTM! Manual E2E trigger workflow configured correctly.

The run-e2e-only workflow allows on-demand E2E test execution via pipeline parameters.


271-294: LGTM! Mainnet E2E gating for npm publish configured correctly.

The workflow properly requires mainnet-e2e-tests to pass before publishing to npm, ensuring production-quality releases.

Comment on lines 127 to 136
SLACK_MESSAGE=":page_facing_up: *E2E Test Report*\n\
*Project:* ${CIRCLE_PROJECT_REPONAME}\n\
*Triggered by:* ${CIRCLE_USERNAME}\n\
*Branch:* ${CIRCLE_BRANCH}\n\
*Commit:* <https://github.com/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}/commit/${CIRCLE_SHA1}|${CIRCLE_SHA1}>\n\
*Job:* <https://circleci.com/gh/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}/${CIRCLE_BUILD_NUM}|View Job>\n\
*Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/mochawesome-report/mochawesome.html|View HTML Report>\n\
\n*Test Summary:*\n\
*Total Suites:* ${TOTAL_SUITES}\n\
*Passes:* ${TOTAL_PASSES}\n\
*Pending:* ${TOTAL_PENDING}\n\
*Failures:* ${TOTAL_FAILURES}\n\
*Start Time:* ${START_TIME_FORMATTED}\n\
*End Time:* ${END_TIME_FORMATTED}\n\
*Duration:* ${DURATION_MIN} minutes"
fi
*Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
curl -X POST -H 'Content-type: application/json' \
--data '{"text":"'"$SLACK_MESSAGE"'"}' $E2E_SLACK_WEBHOOK_URL
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix artifact URL in Slack notification.

The report URL points to the source location /home/circleci/e2e-sdk-modular/reports/test-report.html, but CircleCI artifacts are served from where they're stored. Since line 115 copies the file to /tmp/test-report/, the URL should reference that path.

Apply this diff:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
SLACK_MESSAGE=":page_facing_up: *E2E Test Report*\n\
*Project:* ${CIRCLE_PROJECT_REPONAME}\n\
*Triggered by:* ${CIRCLE_USERNAME}\n\
*Branch:* ${CIRCLE_BRANCH}\n\
*Commit:* <https://github.com/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}/commit/${CIRCLE_SHA1}|${CIRCLE_SHA1}>\n\
*Job:* <https://circleci.com/gh/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}/${CIRCLE_BUILD_NUM}|View Job>\n\
*Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/mochawesome-report/mochawesome.html|View HTML Report>\n\
\n*Test Summary:*\n\
*Total Suites:* ${TOTAL_SUITES}\n\
*Passes:* ${TOTAL_PASSES}\n\
*Pending:* ${TOTAL_PENDING}\n\
*Failures:* ${TOTAL_FAILURES}\n\
*Start Time:* ${START_TIME_FORMATTED}\n\
*End Time:* ${END_TIME_FORMATTED}\n\
*Duration:* ${DURATION_MIN} minutes"
fi
*Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
curl -X POST -H 'Content-type: application/json' \
--data '{"text":"'"$SLACK_MESSAGE"'"}' $E2E_SLACK_WEBHOOK_URL
SLACK_MESSAGE=":page_facing_up: *E2E Test Report*\n\
*Project:* ${CIRCLE_PROJECT_REPONAME}\n\
*Triggered by:* ${CIRCLE_USERNAME}\n\
*Branch:* ${CIRCLE_BRANCH}\n\
*Commit:* <https://github.com/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}/commit/${CIRCLE_SHA1}|${CIRCLE_SHA1}>\n\
*Job:* <https://circleci.com/gh/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}/${CIRCLE_BUILD_NUM}|View Job>\n\
*Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>"
curl -X POST -H 'Content-type: application/json' \
--data '{"text":"'"$SLACK_MESSAGE"'"}' $E2E_SLACK_WEBHOOK_URL
🤖 Prompt for AI Agents
In .circleci/config.yml around lines 127 to 136, the Slack report URL points to
/home/circleci/e2e-sdk-modular/reports/test-report.html but the artifact was
copied to /tmp/test-report/ on line 115; update the Report URL to reference the
correct stored artifact path (/tmp/test-report/test-report.html) in the
SLACK_MESSAGE so the Slack link resolves to the actual CircleCI artifact
location before sending the curl POST.

@github-actions
Copy link

Yooo! You forgot to bump the version in package.json!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (3)
.circleci/config.yml (3)

6-8: Remove or utilize unused top-level test_command pipeline parameter.

The pipeline-level parameter is declared but never consumed by any workflow. Either remove it or reference it in workflows (e.g., test_command: << pipeline.parameters.test_command >>).


141-141: Fix artifact URL in Slack notification.

The report URL still points to the old source location /home/circleci/e2e-sdk-modular/reports/test-report.html. Since the artifact is stored at /tmp/test-report/, the URL must reference that path.

Apply this diff:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>"

271-278: testnet-e2e-tests job filter points to non-existent develop branch.

The job is configured to run only on develop, which currently doesn't exist per the PR objectives. Create the develop branch or update this filter to a valid branch (e.g., main) so E2E tests execute as expected.

🧹 Nitpick comments (1)
.circleci/config.yml (1)

146-207: Consider removing commented-out legacy code.

The commented-out section spans 60+ lines and appears to be a previous implementation of test result parsing and Slack notifications. If this code is no longer needed, remove it to improve maintainability. If it's being kept for reference or potential future use, consider adding a comment explaining why.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f82f83 and d77df75.

📒 Files selected for processing (1)
  • .circleci/config.yml (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (7)
.circleci/config.yml (7)

99-105: LGTM! Expanded environment setup.

The additional sed replacements for primary/secondary keys, wallet addresses, paymaster, and sponsor correctly prepare the E2E environment with all necessary credentials.


110-110: LGTM! Parameterized test command execution.

The test command now correctly uses the job parameter << parameters.test_command >>, enabling different test suites per workflow.


112-118: LGTM! Useful debugging step.

The aggregated report debugging will help troubleshoot test result parsing issues.


122-129: LGTM! Artifact path updated consistently.

The artifact directory is correctly updated to /tmp/test-report in both the copy step and store_artifacts.


258-263: LGTM! New workflow for manual E2E testing.

The run-e2e-only workflow correctly uses the pipeline parameter to enable on-demand E2E test execution with the all-chains command.


279-286: LGTM! mainnet E2E tests correctly configured.

The mainnet-e2e-tests job runs the appropriate test suite on the master branch.


287-294: LGTM! Publishing gated on mainnet E2E tests.

The dependency correctly ensures that mainnet E2E tests must pass before publishing to npm, improving release quality.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (3)
.circleci/config.yml (3)

6-8: Top-level test_command parameter remains unused.

The pipeline parameter test_command is declared but line 264 hardcodes "npm run test-all-chains" instead of referencing << pipeline.parameters.test_command >>. Either remove lines 6-8 or update line 264 to consume the parameter.


132-146: Slack report URL points to wrong artifact path.

Line 142 references /home/circleci/e2e-sdk-modular/reports/test-report.html, but artifacts are served from where they're stored (/tmp/test-report/ per line 129). The Slack link will be broken.

Apply this diff:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>"

272-279: Testnet E2E job filtered to non-existent develop branch.

The testnet-e2e-tests job is configured to run only on the develop branch, which doesn't exist in the repository (confirmed by 404 in past review verification). This job will never execute until the branch is created or the filter is updated to an existing branch.

If the develop branch is planned for future use, consider documenting this. Otherwise, update the filter to an existing branch:

           filters:
             branches:
               only:
-                - develop
+                - master
🧹 Nitpick comments (1)
.circleci/config.yml (1)

147-208: Consider removing commented-out test result parsing code.

Lines 147-208 contain a large block of commented-out code for detailed test result parsing. Since the simplified notification approach (lines 132-145) is now active, consider removing this dead code to reduce clutter unless you plan to re-enable it soon.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d77df75 and 41ea9d3.

📒 Files selected for processing (1)
  • .circleci/config.yml (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (6)
.circleci/config.yml (6)

50-53: LGTM! Job parameter properly defined.

The test_command parameter is correctly defined and consumed at line 111, allowing different workflows to run different test suites.


90-111: LGTM! E2E setup and execution properly parameterized.

The expanded environment variable substitutions (lines 101-106) and parameterized test command execution (line 111) are correctly implemented.


113-127: LGTM! Debug step and artifact relocation are correctly implemented.

The debug output (lines 114-119) will help troubleshoot test execution, and the artifact path has been properly updated to /tmp/test-report.


128-131: LGTM! Artifact path correctly updated.

The store_artifacts path (line 129) properly references /tmp/test-report, consistent with the copy operation at line 124.


259-265: LGTM! Workflow for manual/scheduled E2E runs is correctly configured.

The run-e2e-only workflow properly uses the run_e2e_tests pipeline parameter to trigger E2E tests with the all-chains command. This supports the nightly scheduled runs mentioned in PR objectives.


280-295: LGTM! Mainnet E2E integration is correctly configured.

The mainnet-e2e-tests job (lines 280-287) runs the correct test command on master, and the publish-npm-package job (lines 288-295) properly requires it to pass before publishing. This ensures packages are only published after mainnet E2E tests succeed.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
.circleci/config.yml (2)

274-281: Review branch filters for testnet E2E tests.

The testnet-e2e-tests job is filtered to run on develop and PRO-3351-E2e branches. According to the PR objectives, the develop branch doesn't currently exist, so tests won't execute there until it's created. Additionally, PRO-3351-E2e appears to be a PR-specific branch that should likely be removed before merging to avoid running testnet E2E tests on every commit to this feature branch going forward.

#!/bin/bash
# Verify branch existence
echo "Checking develop branch..."
gh api repos/etherspot/etherspot-modular-sdk/branches/develop 2>&1 | head -5

echo -e "\nChecking PRO-3351-E2e branch..."
gh api repos/etherspot/etherspot-modular-sdk/branches/PRO-3351-E2e 2>&1 | head -5

143-143: Fix artifact URL to reference stored location.

The report URL points to the source path /home/circleci/e2e-sdk-modular/reports/test-report.html, but CircleCI serves artifacts from where they're stored via store_artifacts (line 130: /tmp/test-report). Update the URL to reference the artifact storage location.

Apply this diff:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>"
🧹 Nitpick comments (2)
.circleci/config.yml (2)

148-209: Update artifact paths in commented code before re-enabling.

The commented test result parsing logic still references the old /tmp/mochawesome-report path at lines 152, 188, and 198. If you plan to re-enable this functionality, update these paths to /tmp/test-report.


53-53: Clarify default test command value.

The job parameter default is "npm run test-testnet-viem-combined" (line 53), but testnet-e2e-tests explicitly uses "npm run test-all-chains" (line 276). Consider whether the default should be updated to match the most common usage, or if this discrepancy is intentional.

Also applies to: 276-276

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 41ea9d3 and 5c9ffac.

📒 Files selected for processing (1)
  • .circleci/config.yml (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (11)
.circleci/config.yml (11)

6-8: Pipeline parameter is now properly utilized.

The previous concern about the unused test_command pipeline parameter is resolved—it's now consumed by the run-e2e-only workflow at line 265.


50-53: LGTM!

The job parameter allows flexible test command configuration. The default aligns with testnet usage.


90-90: Verify hardcoded e2e repository branch.

The e2e repository is always checked out to master, regardless of which branch triggered the workflow. Ensure this is intentional—if testnet-e2e-tests and mainnet-e2e-tests require different e2e test versions, you may need to parameterize this checkout.


100-106: LGTM!

The additional environment variable substitutions follow the established pattern and expand test configuration capabilities.


111-111: LGTM!

Parameterizing the test command enables different workflows to execute different test suites.


113-120: LGTM!

The debug step provides useful diagnostics for test execution and correctly reads from the source report location.


122-127: LGTM!

The artifact staging step correctly prepares the test report for CircleCI storage.


129-131: LGTM!

The artifact storage path correctly references the new location.


260-265: LGTM!

The run-e2e-only workflow enables on-demand E2E test execution via pipeline parameters.


282-289: LGTM!

The mainnet-e2e-tests job configuration aligns with PR objectives—running mainnet tests on commits to master.


290-297: LGTM!

Requiring mainnet-e2e-tests before publishing ensures mainnet tests pass, which is appropriate for releases from master.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
.circleci/config.yml (3)

6-8: Top-level test_command parameter remains unused.

The pipeline-level test_command parameter is still not consumed by any workflow. Workflows pass string literals directly to the job's test_command parameter (lines 265, 276, 285) rather than referencing << pipeline.parameters.test_command >>. Either remove this parameter or leverage it in your workflows to avoid configuration drift.


133-147: Fix artifact URL in Slack notification - still points to wrong path.

The report URL (line 143) points to /home/circleci/e2e-sdk-modular/reports/test-report.html, but CircleCI serves artifacts from where they're stored via store_artifacts (line 130: /tmp/test-report/). This link will be broken.

Apply this diff:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/tmp/test-report/test-report.html|View HTML Report>"

274-281: testnet-e2e-tests references non-existent develop branch and temporary PR branch.

The develop branch doesn't exist (confirmed 404 in past review), so tests filtered to it will never run. Additionally, including the PR branch PRO-3351-E2e (line 281) in the configuration is unusual and creates maintenance debt—it should be removed before merging.

Apply this diff to remove both problematic branch filters:

       - e2e-tests:
           name: testnet-e2e-tests
           context: general-vars
           test_command: "npm run test-all-chains"
-          filters:
-            branches:
-              only:
-                - develop
-                - PRO-3351-E2e

Or create the develop branch and remove PRO-3351-E2e:

           filters:
             branches:
               only:
                 - develop
-                - PRO-3351-E2e
🧹 Nitpick comments (3)
.circleci/config.yml (3)

50-53: Consider aligning default values or documenting the difference.

The job parameter test_command defaults to "npm run test-testnet-viem-combined" while the unused pipeline parameter defaults to "npm run test-all-chains". This inconsistency may cause confusion. If both parameters are retained, document why they differ; otherwise, align them or remove the unused pipeline parameter.


148-209: Remove or update commented test result parsing logic.

This extensive commented block contains outdated artifact paths (/tmp/mochawesome-report/) that would fail if re-enabled. Either remove this dead code or update the paths to match the current /tmp/test-report/ structure before uncommenting.


276-276: Consider renaming test command for testnet E2E tests.

The testnet E2E tests use "npm run test-all-chains" (line 276), which doesn't clearly indicate testnet-specific execution. Per PR objectives, this should likely be "npm run test-testnet-viem-combined" to match the stated intent.

Apply this diff:

       - e2e-tests:
           name: testnet-e2e-tests
           context: general-vars
-          test_command: "npm run test-all-chains"
+          test_command: "npm run test-testnet-viem-combined"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c9ffac and 920223a.

📒 Files selected for processing (1)
  • .circleci/config.yml (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (8)
.circleci/config.yml (8)

90-90: Verify hardcoded master branch checkout.

The e2e repository is always checked out from master regardless of which branch triggered the test. Confirm this is intentional; if different branches need different e2e test versions, parameterize the branch or use a matching branch strategy.


100-106: LGTM! Environment variable expansion looks good.

The additional sed replacements for session keys, wallet addresses, paymaster, and sponsor address are correctly formatted and expand the test environment setup appropriately.


111-111: LGTM! Test command parameterization implemented correctly.

The use of << parameters.test_command >> properly consumes the job parameter, enabling flexible test execution across workflows.


113-120: LGTM! Helpful debug step for troubleshooting.

The aggregated report inspection with jq fallback provides useful debugging information when tests fail. The when: always ensures visibility even on failures.


122-131: LGTM! Artifact path migration completed correctly.

The test report is properly copied to /tmp/test-report/ and stored as an artifact with consistent paths throughout this section.


260-266: Verify test command alignment with PR objectives.

The run-e2e-only workflow runs "npm run test-all-chains", but PR objectives state nightly runs should execute test-mainnet-viem-combined. Confirm whether this command is correct or if it should match the stated objective.


283-289: LGTM! Mainnet E2E configuration aligns with PR objectives.

The mainnet E2E tests correctly run test-mainnet-viem-combined on the master branch, matching the stated PR objectives for commits to master.


290-297: LGTM! Publish dependencies correctly updated.

The publish-npm-package job now correctly depends on mainnet-e2e-tests (line 293) instead of the generic e2e-tests, ensuring mainnet tests pass before publishing.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (4)
.circleci/config.yml (4)

6-8: Remove or utilize unused top-level test_command pipeline parameter.

This issue remains unresolved from the previous review. The pipeline-level parameter test_command is declared but never consumed by any workflow, creating unnecessary configuration overhead.


5-5: Critical: Default run_e2e_tests: true still breaks standard CI/CD workflow.

This issue remains unresolved from the previous review. With run_e2e_tests defaulting to true, the install-and-publish workflow (lines 261-262) will never run by default, blocking normal commits from triggering installation, tests, and publishing.


126-139: Fix artifact URL in Slack notification - issue remains unresolved.

This issue from the previous review remains unresolved. The report URL at line 136 incorrectly references the source path /home/circleci/e2e-sdk-modular/reports/test-report.html. CircleCI serves artifacts from the destination path specified in store_artifacts (line 123: test-report), not from the source path.

Apply this diff:

-                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/home/circleci/e2e-sdk-modular/reports/test-report.html|View HTML Report>"
+                *Report:* <https://output.circle-artifacts.com/output/job/${CIRCLE_WORKFLOW_JOB_ID}/artifacts/${CIRCLE_NODE_INDEX}/test-report/test-report.html|View HTML Report>"

267-274: testnet-e2e-tests job issues remain unresolved.

Two issues:

  1. develop branch doesn't exist (duplicate from previous review): The job filters on develop branch which doesn't exist per PR objectives, so testnet E2E tests will never run until the branch is created.

  2. Remove temporary PR branch from filters: Line 274 includes PRO-3351-E2e (the current PR branch), which should not be part of the permanent configuration. Remove this before merging.

Apply this diff:

           filters:
             branches:
               only:
                 - develop
-                - PRO-3351-E2e
🧹 Nitpick comments (2)
.circleci/config.yml (2)

50-53: Clarify the default value for test_command parameter.

The default value "npm run test-testnet-viem-combined" doesn't match any of the actual invocations in the workflows (which use either "npm run test-all-chains" or "npm run test-mainnet-viem-combined"). Consider updating the default to a more commonly used value or documenting the rationale for this specific default.


141-202: Consider removing or documenting commented code block.

This large commented section contains legacy mochawesome-based reporting logic. If it's kept for future reference, consider adding a comment explaining why it's preserved. Otherwise, removing it would improve config clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 920223a and 9c60cc9.

📒 Files selected for processing (1)
  • .circleci/config.yml (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (6)
.circleci/config.yml (6)

90-90: Verify intentional hard-coding to master branch in e2e repo.

The e2e test repository is always checked out to master regardless of which branch (develop/master) triggered the workflow. Confirm this is intentional, as you may want to sync branch selection between the SDK repo and the e2e test repo for consistency.


100-106: LGTM: Environment variable substitutions expanded.

The additional sed commands for session keys, multiple wallet addresses, and paymaster/sponsor addresses follow the established pattern and properly support the expanded test scenarios.


111-111: LGTM: Parameterized test command execution.

The test command now properly uses the job parameter, enabling flexible test execution across different workflows and branches.


253-258: LGTM: run-e2e-only workflow structure is correct.

The workflow is properly configured to run E2E tests when explicitly triggered. The issue is with the pipeline parameter default at line 5, which causes this workflow to run by default instead of being opt-in.


283-298: LGTM: Publish workflows correctly updated.

The publish workflows now properly depend on mainnet-e2e-tests and maintain master branch filtering, ensuring that only mainnet-validated changes trigger npm and GitHub releases.


121-123: Verify artifact path consistency. I didn’t find any test config declaring reports output at /home/circleci/e2e-sdk-modular/reports. Please confirm the test framework writes to this directory (or update the path in CircleCI or your test config) so artifacts are correctly collected.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

♻️ Duplicate comments (2)
.circleci/config.yml (2)

5-5: Critical: Default run_e2e_tests: true breaks standard CI/CD workflow.

With run_e2e_tests defaulting to true, the install-and-publish workflow (line 337: when: not: << pipeline.parameters.run_e2e_tests >>) will never run by default. Normal commits won't trigger installation or branch-specific E2E tests.

Apply this diff:

   run_e2e_tests:
     type: boolean
-    default: true
+    default: false

6-8: Remove unused pipeline parameter test_command.

The pipeline-level test_command parameter is never consumed. All workflows pass their own literal test_command values directly to the e2e-tests job (lines 333, 344, 353), so this top-level parameter serves no purpose.

Apply this diff:

   run_e2e_tests:
     type: boolean
     default: true
-  test_command:
-    type: string
-    default: "npm run test-all-chains"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9c60cc9 and 188c766.

📒 Files selected for processing (1)
  • .circleci/config.yml (7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T16:21:31.320Z
Learnt from: chetanbothra
PR: etherspot/etherspot-modular-sdk#66
File: .circleci/config.yml:226-231
Timestamp: 2025-05-28T16:21:31.320Z
Learning: CircleCI scheduled triggers (cron jobs) for nightly runs are configured at the CircleCI application/project level through the web interface, not in the workflow configuration files (.circleci/config.yml). They are not defined within the workflows section of the configuration.

Applied to files:

  • .circleci/config.yml
🔇 Additional comments (6)
.circleci/config.yml (6)

50-53: LGTM! Job parameterization implemented correctly.

The test_command parameter is properly defined and consumed at line 111, enabling flexible test execution across different workflows.

Also applies to: 111-111


101-106: LGTM! Environment variable substitutions expanded.

The additional sed replacements for primary/secondary keys and addresses properly support the expanded E2E test configuration.


206-276: LGTM! Slack reporting paths are consistent.

The JSON file path (line 210) correctly references /home/circleci/e2e-sdk-modular/reports/aggregated-report.json, and the artifact URL (line 236) properly uses the test-report destination that matches the store_artifacts configuration at lines 122-123.


328-333: Verify run-e2e-only workflow is intended for scheduled/manual runs.

This workflow only executes when run_e2e_tests is true. Based on the learnings, this should be triggered via CircleCI project-level scheduled triggers (cron), not by normal commits. Ensure the pipeline parameter is set correctly for scheduled nightly runs in the CircleCI UI.

Based on learnings


350-357: LGTM! Mainnet E2E workflow configured correctly.

The mainnet-e2e-tests job correctly uses npm run test-mainnet-viem-combined as specified in the PR objectives and runs only on the master branch.


358-365: LGTM! Publishing correctly depends on mainnet E2E tests.

The publish-npm-package job now requires mainnet-e2e-tests to pass before publishing, ensuring packages are only published after mainnet tests succeed.

cd ~
git clone https://github.com/etherspot/e2e-sdk-modular.git
cd e2e-sdk-modular
git checkout master
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Hard-coded git checkout master breaks testnet E2E workflow.

The testnet-e2e-tests job is configured to run on the develop branch (lines 346-349), but line 90 forces a checkout of master. This means testnet tests will always run against master code, not the develop branch being tested.

Remove this line entirely - the e2e repo should run from its default branch, or conditionally check out based on which branch triggered the workflow.

                cd e2e-sdk-modular
-               git checkout master
                sed -i.bak "s/private_key/$PRIVATE_KEY/g" .env
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
git checkout master
cd e2e-sdk-modular
sed -i.bak "s/private_key/$PRIVATE_KEY/g" .env
🤖 Prompt for AI Agents
In .circleci/config.yml around line 90, the pipeline forcibly runs "git checkout
master" which causes the testnet-e2e-tests job (configured to run on develop) to
always execute against master; remove the hard-coded checkout so the e2e repo
uses its default/triggering branch, or replace it with a conditional checkout
that checks the CircleCI branch variable (e.g. only checkout master when the
workflow was triggered on master, otherwise checkout the current CIRCLE_BRANCH)
so tests run against the intended branch.

# branches:
# only:
# - master
test_command: "npm run test-all-chains"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Inconsistent test command for testnet E2E.

The PR objectives state that commits to develop should run npm run test-testnet-viem-combined, but line 344 uses npm run test-all-chains. This inconsistency may cause longer test runs than intended or test coverage gaps.

Apply this diff:

       - e2e-tests:
           name: testnet-e2e-tests
           context: general-vars
-          test_command: "npm run test-all-chains"
+          test_command: "npm run test-testnet-viem-combined"
           filters:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test_command: "npm run test-all-chains"
- e2e-tests:
name: testnet-e2e-tests
context: general-vars
test_command: "npm run test-testnet-viem-combined"
filters:
🤖 Prompt for AI Agents
In .circleci/config.yml around line 344 the test_command is set to "npm run
test-all-chains" which conflicts with the PR objective to run the shorter
testnet E2E task; replace the value with "npm run test-testnet-viem-combined" so
the CI triggers the intended test suite, and save the file ensuring no
additional whitespace or quoting changes break YAML parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants