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

Remove memory store and increase test coverage #1011

Merged
merged 9 commits into from
Mar 31, 2022

Conversation

MHassanTariq
Copy link
Contributor

Description

This PR increases the test coverage. It adds test cases that were previously not catered.

For details refer to issue #985

Checklist

  • Code is commented where needed
  • Unit test coverage for new or modified code paths
  • npm run test passes
  • Changelog is updated
  • Tag 1 of @rafaelcr or @zone117x for review

@MHassanTariq MHassanTariq changed the base branch from master to develop January 31, 2022 12:20
@github-actions
Copy link

github-actions bot commented Jan 31, 2022

@github-actions github-actions bot temporarily deployed to pull request January 31, 2022 12:22 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2022

Codecov Report

Merging #1011 (48c8e05) into develop (6fe829c) will increase coverage by 2.62%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #1011      +/-   ##
===========================================
+ Coverage    72.96%   75.59%   +2.62%     
===========================================
  Files           91       90       -1     
  Lines         9900     9563     -337     
  Branches      1928     1897      -31     
===========================================
+ Hits          7224     7229       +5     
+ Misses        2561     2230     -331     
+ Partials       115      104      -11     
Impacted Files Coverage Δ
src/index.ts 0.00% <0.00%> (ø)
src/rosetta-helpers.ts 72.38% <0.00%> (+0.39%) ⬆️
src/api/controllers/db-controller.ts 79.12% <0.00%> (+0.72%) ⬆️
src/datastore/postgres-store.ts 87.63% <0.00%> (+0.76%) ⬆️
src/api/routes/tx.ts 78.33% <0.00%> (+1.11%) ⬆️
src/api/routes/block.ts 90.41% <0.00%> (+15.06%) ⬆️
src/api/routes/status.ts 72.72% <0.00%> (+22.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fe829c...48c8e05. Read the comment docs.

@MHassanTariq MHassanTariq linked an issue Feb 7, 2022 that may be closed by this pull request
@github-actions github-actions bot temporarily deployed to pull request February 7, 2022 12:26 Inactive
@github-actions github-actions bot temporarily deployed to commit February 7, 2022 12:26 Inactive
@MHassanTariq MHassanTariq self-assigned this Feb 8, 2022
@github-actions github-actions bot temporarily deployed to commit February 15, 2022 15:25 Inactive
@MHassanTariq MHassanTariq changed the title [Draft] Increase test coverage Increase test coverage Feb 17, 2022
@MHassanTariq MHassanTariq marked this pull request as ready for review February 17, 2022 08:34
@MHassanTariq
Copy link
Contributor Author

This PR is complete for now. I will make a new PR for rosetta test cases to increase the code coverage.

@MHassanTariq
Copy link
Contributor Author

MHassanTariq commented Feb 22, 2022

@rafaelcr I was trying to rebase this PR. However, one of my tests, here, that I added in this PR has been failing. The error shown is

TypeError: Cannot read properties of undefined (reading 'count')

This error is generated as the length of rows for total is 0.
Can you suggest anything on how to resolve this?

@MHassanTariq MHassanTariq force-pushed the increase_test_coverage branch from 9d7518a to b1deb6d Compare February 24, 2022 12:43
@github-actions github-actions bot temporarily deployed to pull request February 24, 2022 12:45 Inactive
@github-actions github-actions bot temporarily deployed to commit February 24, 2022 12:45 Inactive
@@ -2985,6 +2986,7 @@ export class PgDataStore
`,
[limit, offset]
);
console.log('printing results here: ', results);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove these debug logs?

src/index.ts Outdated
@@ -141,11 +140,6 @@ async function init(): Promise<void> {
db = OfflineDummyStore;
} else {
switch (process.env['STACKS_BLOCKCHAIN_API_DB']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This env variable is now unused. Can you remove it from the entire project?

@@ -12,9 +12,11 @@ export default async (): Promise<void> => {
process.env.NODE_ENV = 'test';
}
loadDotEnv();
process.env.PG_DATABASE = 'postgres';
const db = await PgDataStore.connect({ skipMigrations: true, usageName: 'setup' });
const server = await startEventServer({
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not mistaken, this entire global server is never used... Can you try removing it (also removing the db) from here and teardown.ts to see if tests are at all affected? If not, we should delete it.

@github-actions github-actions bot temporarily deployed to pull request March 24, 2022 11:34 Inactive
@github-actions github-actions bot temporarily deployed to commit March 24, 2022 11:34 Inactive
@@ -2974,6 +2974,7 @@ export class PgDataStore
const total = await client.query<{ count: number }>(`
SELECT block_count AS count FROM chain_tip
`);
console.log('printing total rows here: ', total.rows);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this one too

Comment on lines 12 to 14
if (!process.env.NODE_ENV) {
process.env.NODE_ENV = 'test';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines should be kept

Comment on lines 12 to 14
if (!process.env.NODE_ENV) {
process.env.NODE_ENV = 'test';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep these lines too

Comment on lines 12 to 14
if (!process.env.NODE_ENV) {
process.env.NODE_ENV = 'test';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep this

@@ -27,7 +27,7 @@ type TableCellValue = string | number | bigint | undefined;
* @param blockHeight Specific block height at which to query balances
*/
async function printTopAccountBalances(count: number, blockHeight: number) {
const db = await PgDataStore.connect(true);
const db = await PgDataStore.connect({ skipMigrations: true, usageName: 'tests' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const db = await PgDataStore.connect({ skipMigrations: true, usageName: 'tests' });
const db = await PgDataStore.connect({ skipMigrations: true, usageName: 'utils' });

Copy link
Collaborator

@rafaelcr rafaelcr left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Before merging, can you change the PR's title to "Remove memory store and increase test coverage"?

@MHassanTariq MHassanTariq force-pushed the increase_test_coverage branch from 8d3994f to 48c8e05 Compare March 31, 2022 09:22
@MHassanTariq MHassanTariq changed the title Increase test coverage Remove memory store and increase test coverage Mar 31, 2022
@github-actions github-actions bot temporarily deployed to commit March 31, 2022 09:25 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 31, 2022 09:25 Inactive
@MHassanTariq MHassanTariq requested a review from rafaelcr March 31, 2022 09:49
@rafaelcr rafaelcr merged commit cacf9a6 into develop Mar 31, 2022
@rafaelcr rafaelcr deleted the increase_test_coverage branch March 31, 2022 17:27
@MHassanTariq MHassanTariq mentioned this pull request Apr 8, 2022
9 tasks
@blockstack-devops
Copy link

🎉 This PR is included in version 4.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@blockstack-devops
Copy link

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase test code coverage to 80%
4 participants