cerc-io/ipld-eth-server
17
0
Fork 1
Code Issues 10 Pull Requests 1 Actions Packages Projects Releases 37 Wiki Activity

integration test #53

Merged
ramilexe merged 32 commits from integration_tests into endpoints 2021-06-18 06:59:19 +00:00
Conversation 1 Commits 32 Files Changed 34 +29096 -174
ramilexe commented 2021-04-16 13:57:46 +00:00 (Migrated from github.com)
Copy Link
No description provided.
ashwinphatak (Migrated from github.com) reviewed 2021-06-04 04:22:20 +00:00
pkg/eth/api.go
@ -645,3 +695,3 @@
storageVal, err := pea.B.GetStorageByNumberOrHash(ctx, address, common.HexToHash(key), blockNrOrHash)
if storageVal != nil && err == nil {
return storageVal, nil
var value common.Hash
ashwinphatak (Migrated from github.com) commented 2021-06-04 04:16:56 +00:00
Copy Link

Did you confirm this behaviour against the standard JSON RPC API endpoint, i.e. expected result when the storage slot doesn't exist.

Did you confirm this behaviour against the standard JSON RPC API endpoint, i.e. expected result when the storage slot doesn't exist.
pkg/eth/api_test.go
ashwinphatak (Migrated from github.com) commented 2021-06-04 04:18:13 +00:00
Copy Link

Add a comment explaining this value?

Add a comment explaining this value?
pkg/eth/api_test.go
ashwinphatak (Migrated from github.com) commented 2021-06-04 04:19:56 +00:00
Copy Link

How are we confirming expected behaviour (same question for all new assertions)?

How are we confirming expected behaviour (same question for all new assertions)?
i-norden reviewed 2021-06-04 05:01:50 +00:00
pkg/eth/api.go
@ -645,3 +695,3 @@
storageVal, err := pea.B.GetStorageByNumberOrHash(ctx, address, common.HexToHash(key), blockNrOrHash)
if storageVal != nil && err == nil {
return storageVal, nil
var value common.Hash
i-norden commented 2021-06-04 05:01:50 +00:00
Member
Copy Link

Yeah I think this is undoing one of the fixes Ramil made here, the unit test is broken in what it is expecting rather than what it is receiving in this case (and probably all the cases where the unit tests were broken by the changes made in this PR, as the unit tests were passing before them). The majority of the problems this PR is fixing are due to me not anticipating (and failing to test) what the correct expected results were for error and nil cases. Ramil's integration tests compare the results to the results from a standard ETH JSON-RPC client so those are the source of truth.

Yeah I think this is undoing one of the fixes Ramil made here, the unit test is broken in what it is expecting rather than what it is receiving in this case (and probably all the cases where the unit tests were broken by the changes made in this PR, as the unit tests were passing before them). The majority of the problems this PR is fixing are due to me not anticipating (and failing to test) what the correct expected results were for error and nil cases. Ramil's integration tests compare the results to the results from a standard ETH JSON-RPC client so those are the source of truth.
arijitAD (Migrated from github.com) reviewed 2021-06-04 05:03:30 +00:00
pkg/eth/api.go
@ -645,3 +695,3 @@
storageVal, err := pea.B.GetStorageByNumberOrHash(ctx, address, common.HexToHash(key), blockNrOrHash)
if storageVal != nil && err == nil {
return storageVal, nil
var value common.Hash
arijitAD (Migrated from github.com) commented 2021-06-04 05:03:30 +00:00
Copy Link

Yes, I confirmed the behavior manually.
We have two tests suite:
Unit test: Which makes calls to the DB and checks expectations.
Integration test: Which compares ETH and IPLD APIs response.
I will check if there is an Integration test for the same or else I will add it.

Yes, I confirmed the behavior manually. We have two tests suite: **Unit test:** Which makes calls to the DB and checks expectations. **Integration test**: Which compares ETH and IPLD APIs response. I will check if there is an Integration test for the same or else I will add it.
arijitAD (Migrated from github.com) reviewed 2021-06-04 05:04:36 +00:00
pkg/eth/api_test.go
arijitAD (Migrated from github.com) commented 2021-06-04 05:04:36 +00:00
Copy Link

https://github.com/vulcanize/ipld-eth-server/pull/53#discussion_r645290501

https://github.com/vulcanize/ipld-eth-server/pull/53#discussion_r645290501
arijitAD (Migrated from github.com) reviewed 2021-06-10 03:21:12 +00:00
pkg/eth/api_test.go
arijitAD (Migrated from github.com) commented 2021-06-10 03:21:11 +00:00
Copy Link

Done

Done
ashwinphatak (Migrated from github.com) approved these changes 2021-06-11 05:24:12 +00:00
docker-compose.yml
@ -22,3 +22,3 @@
- db
image: vulcanize/statediff-migrations:v0.3.0
image: vulcanize/statediff-migrations:v0.4.0
environment:
ashwinphatak (Migrated from github.com) commented 2021-06-11 05:15:15 +00:00
Copy Link

Check if this is the latest one?

Check if this is the latest one?
test/README.md
@ -0,0 +1,15 @@
Spin up services:
ashwinphatak (Migrated from github.com) commented 2021-06-11 05:22:34 +00:00
Copy Link

Add documentation on how to run the unit and integration tests.

Add documentation on how to run the unit and integration tests.
arijitAD (Migrated from github.com) reviewed 2021-06-14 04:57:40 +00:00
test/README.md
@ -0,0 +1,15 @@
Spin up services:
arijitAD (Migrated from github.com) commented 2021-06-14 04:57:40 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-06-14 04:58:10 +00:00
docker-compose.yml
@ -22,3 +22,3 @@
- db
image: vulcanize/statediff-migrations:v0.3.0
image: vulcanize/statediff-migrations:v0.4.0
environment:
arijitAD (Migrated from github.com) commented 2021-06-14 04:58:10 +00:00
Copy Link

The test fails after upgrading the docker images.

The test fails after upgrading the docker images.
ashwinphatak (Migrated from github.com) reviewed 2021-06-14 05:02:04 +00:00
test/README.md
ashwinphatak (Migrated from github.com) commented 2021-06-14 05:02:04 +00:00
Copy Link

Typo ("intrgration")

Typo ("intrgration")
arijitAD (Migrated from github.com) reviewed 2021-06-14 05:25:46 +00:00
test/README.md
arijitAD (Migrated from github.com) commented 2021-06-14 05:25:46 +00:00
Copy Link

Done

Done
i-norden approved these changes 2021-06-14 20:16:47 +00:00
i-norden left a comment
Member
Copy Link

Looks great! Left a few nitpicky comments, otherwise this is good to merge.

Looks great! Left a few nitpicky comments, otherwise this is good to merge.
docker-compose.yml
@ -22,3 +22,3 @@
- db
image: vulcanize/statediff-migrations:v0.3.0
image: vulcanize/statediff-migrations:v0.4.0
environment:
i-norden commented 2021-06-14 19:43:46 +00:00
Member
Copy Link

Latest is v0.5.0

Latest is v0.5.0
pkg/eth/backend_utils.go
i-norden commented 2021-06-14 19:52:23 +00:00
Member
Copy Link

Should move this dep into the section below

Should move this dep into the section below
pkg/eth/backend_utils.go
@ -244,23 +244,91 @@ func newRPCTransactionFromBlockIndex(b *types.Block, index uint64) *RPCTransacti
}
i-norden commented 2021-06-14 19:59:43 +00:00
Member
Copy Link

This continue doesn't need the label

This continue doesn't need the label
pkg/eth/ipld_retriever.go
@ -430,3 +430,2 @@
// RetrieveStorageAtByAddressAndStorageKeyAndBlockHash returns the cid and rlp bytes for the storage value corresponding to the provided address, storage key, and block hash
func (r *IPLDRetriever) RetrieveStorageAtByAddressAndStorageKeyAndBlockHash(address common.Address, storageLeafKey, hash common.Hash) (string, []byte, error) {
// RetrieveStorageAtByAddressAndStorageSlotAndBlockHash returns the cid and rlp bytes for the storage value corresponding to the provided address, storage slot, and block hash
i-norden commented 2021-06-14 20:14:49 +00:00
Member
Copy Link

The name of this function implies the actual storage leaf key is provided to it, it shouldn't need to hash it again. If an actual storage key was provided this hashing would break the function. We should perform the hashing of the slot key into the leaf key at the level above or change the name to RetrieveStorageAtByAddressAndStorageSlotAndBlockHash so that the name aligns with the behavior.

The name of this function implies the actual storage leaf key is provided to it, it shouldn't need to hash it again. If an actual storage key was provided this hashing would break the function. We should perform the hashing of the slot key into the leaf key at the level above or change the name to `RetrieveStorageAtByAddressAndStorageSlotAndBlockHash` so that the name aligns with the behavior.
arijitAD (Migrated from github.com) reviewed 2021-06-18 06:43:30 +00:00
pkg/eth/backend_utils.go
arijitAD (Migrated from github.com) commented 2021-06-18 06:43:29 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-06-18 06:43:35 +00:00
pkg/eth/backend_utils.go
@ -244,23 +244,91 @@ func newRPCTransactionFromBlockIndex(b *types.Block, index uint64) *RPCTransacti
}
arijitAD (Migrated from github.com) commented 2021-06-18 06:43:35 +00:00
Copy Link

Done

Done
arijitAD (Migrated from github.com) reviewed 2021-06-18 06:44:00 +00:00
pkg/eth/ipld_retriever.go
@ -430,3 +430,2 @@
// RetrieveStorageAtByAddressAndStorageKeyAndBlockHash returns the cid and rlp bytes for the storage value corresponding to the provided address, storage key, and block hash
func (r *IPLDRetriever) RetrieveStorageAtByAddressAndStorageKeyAndBlockHash(address common.Address, storageLeafKey, hash common.Hash) (string, []byte, error) {
// RetrieveStorageAtByAddressAndStorageSlotAndBlockHash returns the cid and rlp bytes for the storage value corresponding to the provided address, storage slot, and block hash
arijitAD (Migrated from github.com) commented 2021-06-18 06:43:59 +00:00
Copy Link

Updated it to RetrieveStorageAtByAddressAndStorageSlotAndBlockHash since the api gives us storage slot.

Updated it to `RetrieveStorageAtByAddressAndStorageSlotAndBlockHash` since the api gives us storage slot.
arijitAD commented 2021-06-18 06:55:36 +00:00 (Migrated from github.com)
Copy Link

The tests fail after updating the docker images. Created an issue for this https://github.com/vulcanize/ipld-eth-server/issues/73

The tests fail after updating the docker images. Created an issue for this https://github.com/vulcanize/ipld-eth-server/issues/73
👀 1
Sign in to join this conversation.
Reviewers
No reviewers
i-norden
ashwinphatak
Labels
Clear labels   
bug

Something isn't working

  
critical

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
Epic

   
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
Integration tests

   
invalid

This doesn't seem right

  
question

Further information is requested

  
v5

   
wontfix

This will not be worked on

  
Copied from Github

An issue or PR manually copied from GitHub.

  
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
bug
critical
documentation
duplicate
enhancement
Epic
good first issue
help wanted
Integration tests
invalid
question
v5
wontfix
Copied from Github
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/ipld-eth-server#53
No description provided.
Delete Branch "integration_tests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?