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

Add Vitest testing to the CI pipeline #13

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

ilamanov
Copy link
Contributor

@ilamanov ilamanov commented Jan 22, 2025

Also updates the test files which were failing due to recent rename to EVM files.
Adds SVM tests as well

Copy link
Contributor Author

ilamanov commented Jan 22, 2025

@ilamanov ilamanov force-pushed the 01-21-add_vitest_testing_to_the_ci_pipeline branch 3 times, most recently from 5650a12 to 2368520 Compare January 22, 2025 02:34
DUNE_API_KEY=test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to have some value here because otherwise the hooks return early when the api key is not set. We are mocking all requests so the actual API key is not needed

@ilamanov ilamanov marked this pull request as ready for review January 22, 2025 02:37
@ilamanov ilamanov force-pushed the 01-21-add_vitest_testing_to_the_ci_pipeline branch 2 times, most recently from 77509a9 to fc891e1 Compare January 22, 2025 02:58
@ilamanov ilamanov requested a review from GarrettJMU January 22, 2025 21:52
Copy link
Contributor

@GarrettJMU GarrettJMU left a comment

Choose a reason for hiding this comment

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

Small comment around isAddress

@@ -4,6 +4,7 @@ import { TokenBalancesParams, BalanceData, FetchError } from "./types";
import { fetchSvmBalances } from "./duneApi";
import { useDeepMemo } from "../useDeepMemo";
import { useGetApiKey } from "../provider";
import { isAddress } from "viem";
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

@@ -30,7 +31,7 @@ export const useSvmTransactions = (

// Function to fetch data for a specific page
const fetchDataAsync = async (offset: string | null) => {
if (!walletAddress) return;
if (!apiKey || !walletAddress || !isAddress(walletAddress)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

will this work for svm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this kinda got lost during the big copy paste:( I removed it

@ilamanov ilamanov force-pushed the 01-21-add_vitest_testing_to_the_ci_pipeline branch from fc891e1 to f19602c Compare January 23, 2025 14:46
Copy link
Contributor

@GarrettJMU GarrettJMU left a comment

Choose a reason for hiding this comment

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

Same note here - we prob need to update readme but looks great

@ilamanov
Copy link
Contributor Author

Same note here - we prob need to update readme but looks great

Updated in #16

Copy link
Contributor Author

ilamanov commented Jan 23, 2025

Merge activity

  • Jan 23, 10:24 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 23, 10:24 AM EST: A user merged this pull request with Graphite.

@ilamanov ilamanov merged commit 6847152 into main Jan 23, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants