Skip to content
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

test : Use IsType assertions in test #1574

Merged

Conversation

PrathyushaLakkireddy
Copy link
Contributor

@PrathyushaLakkireddy PrathyushaLakkireddy commented Feb 28, 2024

Overview

closes : #1570

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

Summary by CodeRabbit

  • Tests
    • Enhanced testing for both Full and Light Node types to ensure correct object types are created and utilized.
    • Added assertions using require.IsType in various test functions to check object types.
    • Added assertion in TestStartup and TestMempoolDirectly to validate the type of the node object as FullNode.
    • Introduced a new assertion in TestLightClient_Panics to verify the type of ln.
    • Updated TestNewNode to include assertions using require.IsType to validate the types of nodes created, specifically for LightNode and FullNode.

Copy link
Contributor

coderabbitai bot commented Feb 28, 2024

Walkthrough

The modifications introduce the use of require.IsType assertions in various node-related test functions to ensure the correctness of node object types. This change aims to make the tests more expressive and reliable by explicitly checking the types of nodes created or used within the tests, covering both FullNode and LightNode.

Changes

File Path Change Summary
node/full_node_test.go Added require.IsType assertions in TestStartup and TestMempoolDirectly.
node/light_client_test.go Added require.IsType assertion in TestLightClient_Panics.
node/node_test.go Modified TestNewNode to include require.IsType assertions for node types.

Related issues

  • Use IsType assertions in test #1570: The issue suggests using IsType assertions for expressiveness in tests. This PR directly addresses the suggestion by implementing IsType assertions in node-related tests, making it a perfect candidate for linking.

🐇✨
In the realm of code, where tests doth play,
A rabbit hopped, making changes so gay.
"Assert types," it said, with a small cheer,
Ensuring the nodes, in tests, were clear.
🌟📝
Now every node, both light and full,
Is checked with care, and that's no bull.
🎉🐰

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@PrathyushaLakkireddy PrathyushaLakkireddy marked this pull request as ready for review February 28, 2024 06:20
Copy link
Contributor

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c15fdee and 49ff5a8.
Files selected for processing (3)
  • node/full_node_test.go (1 hunks)
  • node/light_client_test.go (2 hunks)
  • node/node_test.go (1 hunks)
Additional comments: 3
node/light_client_test.go (1)
  • 21-21: Adding the require.IsType assertion in the TestLightClient_Panics function is a positive step towards ensuring that the ln object is indeed a LightNode. This enhances the test's reliability by verifying the object's type before proceeding with the test logic. It's a good practice to include such type checks, especially in dynamic languages where type errors can otherwise go unnoticed until runtime.
node/full_node_test.go (2)
  • 30-31: The addition of the require.IsType assertion in the TestStartup function is a good practice for ensuring that the node object is indeed a FullNode. This type check enhances the test's reliability by verifying the object's type at the beginning of the test. It's a valuable addition to the testing suite, contributing to the overall robustness of the tests.
  • 38-38: Similarly, the inclusion of the require.IsType assertion in the TestMempoolDirectly function is commendable. It ensures that the node object is of the correct type (FullNode) before proceeding with the test logic. This type assertion is crucial for maintaining the integrity and reliability of the tests, especially when dealing with different node types.

node/node_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

Thanks for the submission!

My comment applies to all the places where IsType was used - you still need to remove type assertions, to be able to reach require.IsType in case of an error.

node/full_node_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 49ff5a8 and 461537f.
Files selected for processing (3)
  • node/full_node_test.go (1 hunks)
  • node/light_client_test.go (2 hunks)
  • node/node_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • node/full_node_test.go
  • node/light_client_test.go
  • node/node_test.go

@tzdybal tzdybal self-assigned this Feb 28, 2024
@tzdybal tzdybal added T:enhancement T:testing Related to testing labels Feb 28, 2024
Copy link
Member

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f9907ef and 610d617.
Files selected for processing (3)
  • node/full_node_test.go (1 hunks)
  • node/light_client_test.go (2 hunks)
  • node/node_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • node/full_node_test.go
  • node/light_client_test.go
  • node/node_test.go

@Manav-Aggarwal Manav-Aggarwal added this pull request to the merge queue Feb 28, 2024
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.24%. Comparing base (c9d8467) to head (610d617).
Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1574      +/-   ##
==========================================
+ Coverage   50.23%   52.24%   +2.01%     
==========================================
  Files          52       52              
  Lines        6762     5516    -1246     
==========================================
- Hits         3397     2882     -515     
+ Misses       3024     2308     -716     
+ Partials      341      326      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Merged via the queue into rollkit:main with commit a5c1173 Feb 28, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:enhancement T:testing Related to testing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Use IsType assertions in test
3 participants