-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Problem: fail test is not reported in ci #1622
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1622 +/- ##
===========================================
- Coverage 36.86% 17.72% -19.15%
===========================================
Files 102 72 -30
Lines 8059 5208 -2851
===========================================
- Hits 2971 923 -2048
+ Misses 4710 4162 -548
+ Partials 378 123 -255 |
There was a problem hiding this 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
🧹 Outside diff range and nitpick comments (4)
.github/workflows/build.yml (3)
78-79
: Excellent addition of new test commands.The inclusion of
test-memiavl
andtest-store
commands enhances the test coverage, which is crucial for identifying and reporting test failures. This aligns well with the PR objective of addressing issues with test reporting in CI.Consider adding comments to explain the purpose of each test command for better maintainability.
Line range hint
110-128
: Improved efficiency and robustness in the gomod2nix job.The changes in this section are well-thought-out:
- The modified
if
condition for thecachix/install-nix-action
step improves efficiency.- The new "check working directory is clean" step helps catch unexpected changes.
- The artifact upload and error exit provide better debugging capabilities.
These improvements contribute to a more robust CI process, indirectly supporting the PR objective of better test reporting.
Consider adding a comment explaining the purpose of the
gomod2nix.toml
artifact for future maintainers.
Line range hint
151-167
: Enhanced CI process for contract-related work.The additions to the contracts job are valuable improvements:
- The new test and build steps for contracts ensure proper validation of contract-related changes.
- The "check working directory is clean" step helps catch unexpected modifications in the contracts.
- The artifact upload provides better debugging capabilities for contract-related issues.
These enhancements contribute to a more comprehensive CI process, indirectly supporting the PR objective of improved test reporting.
Consider adding a brief comment explaining the purpose and contents of the
contracts_out
artifact for clarity.Makefile (1)
111-113
: LGTM: Comprehensive updates to test targets and .PHONY declaration.The changes to the
test-versiondb
target and the.PHONY
declaration are well-implemented:
- The addition of the
rocksdb
tag totest-versiondb
appropriately reflects its interaction with RocksDB.- The
.PHONY
declaration has been correctly updated to include all new test targets.These modifications enhance the testing structure and maintain consistency across the Makefile.
Consider adding a comment above the
test-versiondb
target to explain why it requires bothobjstore
androcksdb
tags. This would improve the maintainability of the Makefile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build.yml (1 hunks)
- Makefile (1 hunks)
🔇 Additional comments (4)
.github/workflows/build.yml (1)
Line range hint
1-167
: Overall improvements align with PR objectives, but specific issue needs addressing.The changes made to this workflow file significantly enhance the CI process:
- Additional test commands in the unittest job increase test coverage.
- Improved efficiency and robustness in the gomod2nix job.
- Enhanced CI process for contract-related work.
These improvements contribute to a more comprehensive and reliable CI environment, which indirectly supports the PR objective of better test reporting. However, it's important to note that the specific issue of failed tests not being reported in CI may not be directly addressed by these changes.
To ensure that failed tests are now being reported correctly, please run the following verification script:
This script will help verify that the CI process is now correctly reporting failed tests, addressing the core issue mentioned in the PR objectives.
Makefile (3)
103-103
: LGTM: Consistent tag addition across test targets.The addition of
-tags=objstore
to thetest
target is consistent with the new test targets and aligns with the PR objective of improving test reporting in CI.
104-108
: LGTM: Improved test granularity with new targets.The addition of
test-memiavl
andtest-store
targets is a positive change that allows for more focused testing of specific components. This granularity can help isolate issues and potentially speed up the CI process by allowing parallel execution of tests.The consistent use of
-tags=objstore
across all test targets is commendable.
103-113
: Summary: Excellent improvements to test structure and CI reporting.The changes made to the Makefile align well with the PR objective of improving test reporting in CI. The additions and modifications:
- Introduce more granular testing capabilities with new targets for specific components.
- Maintain consistency in tag usage across test targets.
- Update the
.PHONY
declaration appropriately.These improvements will likely enhance the efficiency of the CI process and make it easier to isolate and address issues. The changes are well-structured and maintain the overall integrity of the Makefile.
766af76
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
For more info
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
test-memiavl
andtest-store
.objstore
androcksdb
tags.