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

added macros for database tests #6

Conversation

saikishore222
Copy link

@saikishore222 saikishore222 commented Jun 18, 2022

added macros for database tests to remove the repetitive test and make it simple using declarative macros. Macros are inserted

in memory.rs,keyvalue.rs.

Co-authored-by : SanthoshAnguluri santhoshanguluri15@gmail.com

Copy link
Owner

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Looks good to me.. Below are some comments.. Great going..

Few more tasks in gen

src/database/keyvalue.rs Outdated Show resolved Hide resolved
src/database/keyvalue.rs Outdated Show resolved Hide resolved
src/database/mod.rs Outdated Show resolved Hide resolved
src/database/mod.rs Outdated Show resolved Hide resolved
Co-authored-by : SanthoshAnguluri <santhoshanguluri15@gmail.com>
Copy link
Owner

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Look good..

Few more nits..

Comment on lines 399 to 400
macro_rules! run_tests_with_constructor {
(getter $fn_name:ident(), tests ( $($x:tt) , + $(,)? )) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
macro_rules! run_tests_with_constructor {
(getter $fn_name:ident(), tests ( $($x:tt) , + $(,)? )) => {
macro_rules! run_tests_with_initialiser {
(init $fn_name:ident(), tests ( $($x:tt) , + $(,)? )) => {

@@ -219,6 +219,7 @@ pub(crate) trait DatabaseUtils: Database {

impl<T: Database> DatabaseUtils for T {}


Copy link
Owner

Choose a reason for hiding this comment

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

One more redundant extra line..

#[cfg(test)]
mod test {

Copy link
Owner

Choose a reason for hiding this comment

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

redundant line..

Co-authored-by: SanthoshAnguluri <santhoshanguluri15@gmail.com>
Copy link
Owner

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Still not sure how but I am seeing two more redundant lines..

Use cargo fmt and cargo clippy before you push changes..

@@ -410,7 +410,7 @@ impl Database for Tree {

impl BatchDatabase for Tree {
type Batch = sled::Batch;

Copy link
Owner

Choose a reason for hiding this comment

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

Redundant line..

@@ -479,6 +479,7 @@ impl ConfigurableDatabase for MemoryDatabase {
}
}


Copy link
Owner

Choose a reason for hiding this comment

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

Redundant line..

Copy link
Owner

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

The tests are failing because you need change populate_db! here too..

let txid = populate_test_db!(

The macro doesn't exist anymore..

Also before committing changes locally run all the tests to make sure nothing is broken..

To get a list of all the CI tests checkout this file..

features:
- default
- minimal
- all-keys
- minimal,esplora
- key-value-db
- electrum
- compact_filters
- esplora,key-value-db,electrum
- compiler
- rpc
- verify
steps:
- name: checkout
uses: actions/checkout@v2
- name: Generate cache key
run: echo "${{ matrix.rust }} ${{ matrix.features }}" | tee .cache_key
- name: cache
uses: actions/cache@v2
with:
path: |
~/.cargo/registry
~/.cargo/git
target
key: ${{ runner.os }}-cargo-${{ hashFiles('.cache_key') }}-${{ hashFiles('**/Cargo.toml','**/Cargo.lock') }}
- name: Set default toolchain
run: rustup default ${{ matrix.rust }}
- name: Set profile
run: rustup set profile minimal
- name: Add clippy
run: rustup component add clippy
- name: Update toolchain
run: rustup update
- name: Build
run: cargo build --features ${{ matrix.features }} --no-default-features
- name: Clippy
run: cargo clippy --all-targets --features ${{ matrix.features }} --no-default-features -- -D warnings
- name: Test
run: cargo test --features ${{ matrix.features }} --no-default-features

It might make sense to make a small shell script with all the cargo tests and run that before pushing changes..

Co-authored-by: SanthoshAnguluri <santhoshanguluri15@gmail.com>
@rajarshimaitra
Copy link
Owner

I pushed all these changes after the comment fix and few more refactors into the PR branch https://github.com/rajarshimaitra/bdk/commits/test-helper-refactor

I am closing this PR, in favor of that.. Its required lot of conflict resolution.. So the merge didn't haven't as clean as I expected..

For further work open PR new on the original branch..

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