chore: fix circle ci build and test on master #1527
Conversation
…nd remove 'since master' flag from test commands
WalkthroughThe changes in this pull request focus on enhancing the build and testing processes within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
requested review from
aimensahnoun,
kevindavee,
leoslr and
yomarion
…'since master' flag and add smart-contracts build step
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.circleci/config.yml (1)
159-161: Consider DRYing up the duplicated smart contract build stepThe smart contract build step is duplicated in both integration test jobs. Consider extracting it into a reusable command like the existing
step_graph_deployandstep_wait_for_node.references: + step_build_smart_contracts: &step_build_smart_contracts + name: 'Build smart-contracts' + command: 'yarn build --scope @requestnetwork/smart-contracts' # ... existing references ... jobs: test-integration-with-smart-contracts: steps: - run: - name: 'Build smart-contracts' - command: 'yarn build --scope @requestnetwork/smart-contracts' + <<: *step_build_smart_contracts test-integration-with-request-node: steps: - run: - name: 'Build smart-contracts' - command: 'yarn build --scope @requestnetwork/smart-contracts' + <<: *step_build_smart_contractsAlso applies to: 194-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(4 hunks)
🔇 Additional comments (2)
.circleci/config.yml (2)
Line range hint 306-349: LGTM! The workflow dependencies are well structured
The workflow configuration ensures that:
- All tests run after the build job
- The next-release job runs only after all tests pass
- The release process is properly gated by the test suite
81-81: Reconsider skipping nx cache as it may impact build performance
The change from --since master to --skip-nx-cache forces a full rebuild of all packages, which could significantly increase build times. While this might temporarily fix the build issues, it's not an optimal long-term solution.
Consider these alternatives:
- Use nx's built-in dependency graph to ensure smart contracts are built first:
nx affected --target=build - Keep incremental builds but explicitly include smart contracts:
yarn build --since master --include @requestnetwork/smart-contracts
Let's analyze the nx configuration to understand the dependency graph:
Also applies to: 280-282
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.circleci/config.yml (2)
63-63: Consider cost implications of xlarge resource classWhile increasing the resource class will help with build performance, it also increases CI costs. Ensure this upgrade is necessary by monitoring resource utilization metrics.
Line range hint
111-117: Consider organizing tests into focused groupsThe current approach of listing individual packages to ignore/include makes maintenance challenging. Consider:
- Organizing packages into test groups (e.g., core, integration, smart-contracts)
- Using package.json workspaces or lerna to define these groups
Also applies to: 168-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(8 hunks)
🔇 Additional comments (2)
.circleci/config.yml (2)
Line range hint 174-185: LGTM! Test results properly configured
The addition of test result storage for ethereum-storage will help with debugging and monitoring test health.
81-81: Verify impact of disabling incremental builds
Skipping Nx cache and building all packages will ensure consistency but may significantly increase build times. Consider:
- Adding comments explaining why cache is skipped
- Only skipping cache for specific packages that require fresh builds
Also applies to: 285-286
✅ Verification successful
Let me try a different approach to verify the impact of disabling incremental builds.
Let me try one final verification to understand the build dependencies and smart contract relationships.
Based on the gathered information, I can now provide a final response:
Build strategy change is justified and necessary
The change from --since master to --skip-nx-cache is intentional and necessary because:
- Smart contract builds are required for integration tests to pass, as evidenced by PR chore: fix circle ci build and test on master #1527 which fixes failing tests due to missing smart contract builds
- The project has proper build dependencies configured in nx.json, ensuring correct build order
- The codebase has extensive smart contract dependencies across multiple packages, making incremental builds risky for contract-dependent tests
The increased build time is an acceptable trade-off for ensuring reliable test execution, especially for smart contract-related changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check average build time difference
echo "Checking recent workflow durations..."
gh api -X GET /repos/RequestNetwork/requestNetwork/actions/workflows -q '.workflows[] | select(.name=="build-and-test") | .id' | \
xargs -I {} gh api -X GET "/repos/RequestNetwork/requestNetwork/actions/workflows/{}/runs?per_page=10" -q \
'.workflow_runs[] | select(.conclusion=="success") | {created_at, updated_at, duration: (fromdateiso8601(updated_at) - fromdateiso8601(created_at))} | .duration'
Length of output: 485
Script:
#!/bin/bash
# Check for any documentation or comments explaining the build strategy
echo "Checking for build-related comments in config and docs..."
rg -i "(\bnx\b.*cache|build.*strategy|incremental.*build)" -g "!{node_modules,dist}/*" -A 2
# Check package.json for build scripts
echo -e "\nChecking build scripts configuration..."
fd "package.json" --type f --exec cat {} \; | jq -r 'select(.scripts.build != null) | .scripts.build'
# Look for previous build configurations
echo -e "\nChecking git history for build command changes..."
git log -p --no-merges -S "yarn build" -- .circleci/config.yml | head -n 50
Length of output: 1223
Script:
#!/bin/bash
# Check for smart contract build dependencies
echo "Checking for smart contract build dependencies..."
rg -l "smart.*contract|solidity|ethereum" -g "!{node_modules,dist}/*" --type ts
# Check for build order dependencies
echo -e "\nChecking for build dependencies in nx.json..."
fd nx.json --type f --exec cat {} \;
# Check for recent build-related issues or PRs
echo -e "\nChecking recent PRs mentioning build issues..."
gh pr list --state all --limit 10 --search "build in:title,body" --json title,body,number
Length of output: 44464
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.circleci/config.yml (2)
81-81: Consider selective package building for non-master branchesWhile building all packages ensures completeness, it may increase build times unnecessarily. Consider:
- Using
--since masterfor feature branches- Only building all packages on master branch
command: | if [[ "$CIRCLE_BRANCH" == "master" ]]; then yarn build else yarn build --since master fiAlso applies to: 86-88
114-121: Optimize test execution strategyThe current configuration has several potential improvements:
- Consider using test groups instead of listing individual packages
- Evaluate if concurrency=2 is optimal for unit tests
- Document why specific packages are excluded/included
Example test group configuration:
references: unit_test_packages: &unit_test_packages - "@requestnetwork/advance-logic" - "@requestnetwork/currency" # ... other packages integration_test_packages: &integration_test_packages - "@requestnetwork/request-node" - "@requestnetwork/ethereum-storage" # ... other packagesAlso applies to: 169-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.circleci/config.yml(7 hunks)
🔇 Additional comments (2)
.circleci/config.yml (2)
63-63: Verify resource class requirements
The increase in resource class from large to xlarge for both build and integration test jobs will provide more computing power, but also increase CI costs. Please ensure this upgrade is necessary by:
- Monitoring resource utilization in previous builds
- Considering alternative optimizations like caching or parallel execution
Also applies to: 156-156
283-285: Document cache skipping rationale
The --skip-nx-cache flag forces rebuilding of all packages, which was previously questioned. While this ensures reliability, it impacts build performance. Consider:
- Adding a comment explaining why cache skipping is necessary
- Making it conditional based on specific scenarios
- Monitoring the impact on build times
- run:
name: 'Build all packages with lerna'
command: |
# Skip cache on next release to ensure all packages are rebuilt
# This prevents issues with smart contract dependencies
yarn build --skip-nx-cache
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
nx.json (1)
8-8: Consider reducing parallel execution limitThe parallel setting of 3 might be too aggressive for CI resources, especially considering the previous build failures. Consider reducing it to 2 to match the unit test concurrency setting.
- "parallel": 3, + "parallel": 2,.circleci/config.yml (1)
169-175: Consider extracting package scopes to a shared variableThe package scope lists are duplicated across jobs. Consider defining them in the references section for better maintainability.
Add to references section:
integration_test_packages: &integration_test_packages - @requestnetwork/request-node - @requestnetwork/ethereum-storage - @requestnetwork/request-client.js - @requestnetwork/smart-contracts - @requestnetwork/payment-detection - @requestnetwork/payment-processor
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.circleci/config.yml(7 hunks).gitignore(1 hunks)nx.json(1 hunks)
🔇 Additional comments (5)
.gitignore (1)
44-46: LGTM!
The cache directory entries are properly configured to support the new nx caching strategy and smart contract builds.
.circleci/config.yml (4)
63-63: Resource upgrade and build command simplification look good
The xlarge resource class should help with build performance, and removing the --since master flag ensures all packages are built.
Also applies to: 81-81
114-121: Test concurrency and package filtering look good
The separation of packages between unit and integration tests with appropriate concurrency settings should help with stability.
283-285: Verify skip-nx-cache impact
The --skip-nx-cache flag in the next-release job will ensure fresh builds, but might increase build time significantly.
This aligns with the previous discussion about skipping nx cache for releases.
86-88: Verify workspace persistence configuration
Ensure that the new cache paths are correctly persisted and restored across jobs.
✅ Verification successful
Let me gather more information about the workspace persistence configuration.
Workspace persistence configuration is correctly set up
The workspace persistence is properly configured:
- The paths being cached (
./.nx-cache,packages/*/dist,packages/*/*.tsbuildinfo) are persisted in thepersist_to_workspacestep - These paths are consistently restored across multiple jobs using
attach_workspaceat the same working directory - The root path and overall workspace configuration follows CircleCI best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the cache paths exist in other jobs
rg -A 5 "attach_workspace:" .circleci/config.yml
Length of output: 1942
Script:
#!/bin/bash
# Check for persist_to_workspace configurations
rg -A 5 "persist_to_workspace:" .circleci/config.yml
Length of output: 207
Problem
Integration tests which requires smart contracts deployed are failing as smart contract packages is not built.
Description of the changes
buildjob andtest-integration-test-smart-contractsjob toxlargeSummary by CodeRabbit
Summary by CodeRabbit