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

feat: add staking e2e tests #319

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Conversation

drohit-cb
Copy link
Contributor

@drohit-cb drohit-cb commented Nov 26, 2024

What changed? Why?

This PR attempts to introduce some basic staking related e2e tests and to integrate with our internal testing workflows.

Plan is to follow up on this and add many more tests around testing different implementations, networks etc.

Have tried to do it in a way where every product/scope can come and add their specific env vars and tests. As a result

  1. Chose to prefix staking related env vars with prefix STAKE.
  2. Made sure all staking related tests sit under a single test suite implemented via a describe section.
  3. Added an explicit npm script test:e2e:stake to be able to run scope specific tests.

Testing

Tested locally and made sure it's running fine in CI.

>> npm run test:e2e:stake

> @coinbase/[email protected] test:e2e:stake
> npx jest --no-cache --testMatch=**/e2e.ts --coverageThreshold '{}' -t Stake

 PASS  src/tests/e2e.ts (7.926 s)
  Coinbase SDK E2E Test
    ○ skipped should be able to access environment variables
    ○ skipped should have created a dist folder for NPM
    ○ skipped should be able to interact with the Coinbase SDK
  Coinbase SDK Stake E2E Test
    ✓ should be able to access environment variables (1 ms)
    Stake: Reward Tests
      ✓ should list shared eth staking rewards via StakingReward.list (607 ms)
      ✓ should list shared eth staking rewards via ExternalAddress (416 ms)
    Stake: Balance Tests
      ✓ should list shared eth staking balances via StakingBalance.list (367 ms)
    Stake: Validator Tests
      ✓ should list validators (654 ms)
      ✓ should fetch a validator (434 ms)
    Stake: Context Tests
      ✓ should return stakeable balances for shared ETH staking (403 ms)
      ✓ should return unstakeable balances for shared ETH staking (429 ms)
      ✓ should return claimable balances for shared ETH staking (424 ms)
      ✓ should return unstakeable balances for Dedicated ETH staking (515 ms)
    Stake: Build Tests
      ✓ should return an unsigned tx for shared ETH staking (1138 ms)

Test Suites: 1 passed, 1 total
Tests:       3 skipped, 11 passed, 14 total
Snapshots:   0 total
Time:        8.125 s
Ran all test suites with tests matching "Stake".

Qualified Impact

@cb-heimdall
Copy link

cb-heimdall commented Nov 26, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@drohit-cb drohit-cb force-pushed the feat/add-staking-e2e-tests branch 2 times, most recently from 26f0b36 to 9da8cc5 Compare November 26, 2024 18:18
@drohit-cb drohit-cb requested a review from alex-stone November 26, 2024 18:25
marcin-cb
marcin-cb previously approved these changes Nov 26, 2024
src/tests/e2e.ts Outdated

expect(rewards).toBeDefined();
// TODO(rohit): Fix code. This should ideally return 20.
expect(rewards.length).toEqual(19);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a backend change that needs to be made?

One thing to note is that this E2E test will run against many different backend versions as we aim for backwards compatibility.

So we typically run our E2E tests against the last N SDK versions (to simulate developers using older SDK versions). The N depends on the particular domain / product guaranteed stability.

So if this will change / be fixed in the context of a backend change, we might want to make this test less specific?

Copy link
Contributor Author

@drohit-cb drohit-cb Nov 26, 2024

Choose a reason for hiding this comment

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

Is this a backend change that needs to be made?

Yup. Need to dig deeper and find what exactly is the issue.

I could change it to be >=19 and <=20 for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok the API might be doing the right thing, but IMO it's confusing for end users.

Anyways, have updated the test to update the window from beginning time of start date to end time of end date. This does yield correct results.

@cb-heimdall cb-heimdall dismissed marcin-cb’s stale review November 26, 2024 19:56

Approved review 2462470776 from marcin-cb is now dismissed due to new commit. Re-request for approval.

@drohit-cb drohit-cb force-pushed the feat/add-staking-e2e-tests branch from b28b12b to ca6068a Compare November 26, 2024 20:59
@drohit-cb drohit-cb changed the base branch from master to v0.11.0 November 26, 2024 23:35
@drohit-cb drohit-cb merged commit a6dc206 into v0.11.0 Nov 26, 2024
6 checks passed
@drohit-cb drohit-cb deleted the feat/add-staking-e2e-tests branch November 26, 2024 23:35
@alex-stone alex-stone mentioned this pull request Nov 27, 2024
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.

4 participants