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

implement DummyDB functionality for tests #441

Merged

Conversation

thdaraujo
Copy link
Contributor

@thdaraujo thdaraujo commented Jun 24, 2022

[fixes #431]

Implements DummyDB functionality to be used on other tests.

@thdaraujo thdaraujo changed the title [WIP] add DummyDB tx storage [WIP] impleent DummyDB functionality for tests Jun 24, 2022
@thdaraujo thdaraujo changed the title [WIP] impleent DummyDB functionality for tests [WIP] implement DummyDB functionality for tests Jun 24, 2022
@thdaraujo thdaraujo force-pushed the ta/implement-dummydb branch from 1056df5 to 0194465 Compare June 24, 2022 21:12
@adlerjohn adlerjohn added enhancement New feature or request fuel-database labels Jun 25, 2022
@@ -166,7 +166,7 @@ pub mod helpers {
/// Dummy coins
pub coins: HashMap<UtxoId, Coin>,
/// Dummy contracts
pub contract: HashSet<ContractId>,
pub contract: HashMap<ContractId, Contract>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming here that this should be a Map and not a Set.

@thdaraujo thdaraujo force-pushed the ta/implement-dummydb branch from a018cfe to bf605ce Compare June 30, 2022 17:43
@thdaraujo thdaraujo force-pushed the ta/implement-dummydb branch from bf605ce to 4554652 Compare June 30, 2022 21:59
@thdaraujo thdaraujo changed the title [WIP] implement DummyDB functionality for tests implement DummyDB functionality for tests Jun 30, 2022
@thdaraujo thdaraujo marked this pull request as ready for review June 30, 2022 22:36
@ControlCplusControlV
Copy link
Contributor

Do you have any tests for this? Or how have you been testing your changes?

@thdaraujo
Copy link
Contributor Author

thdaraujo commented Jul 2, 2022

Do you have any tests for this? Or how have you been testing your changes?

Yep! I do have some tests, but they look a bit repetitive. I was trying to make them more generic, but probably the only way is by using macros.

I'll commit my tests for transactions to show you what I mean @ControlCplusControlV

unreachable!()
let arc = Arc::new(value.clone());

if let Some(previous_tx) = self.data.lock().tx.insert(*key, arc) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: this method will return the old value (the replaced value) that was overwritten. That was my understanding after reading this:
https://github.com/FuelLabs/fuel-storage/blob/62f163dc21c712e2fa8dd9a6a76cf94f82ffc0d0/src/lib.rs#L27

let key = value.id();

//before insert
assert!(!db.contains_key(&key)?);
Copy link
Contributor Author

@thdaraujo thdaraujo Jul 2, 2022

Choose a reason for hiding this comment

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

I would like to reuse all the tests below and only change the db, value and key for the specific implementation. That way, I could remove some duplication.

But not entirely sure how to do it without macros or leveraging generics. Maybe you have a better suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend looking into rstest as we already use it in other areas. The db could be instantiated via fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info!

I added more test cases and removed most of the duplication, and also found a way to to instantiate the db without needing fixtures after all.

Copy link
Member

@Voxelot Voxelot left a comment

Choose a reason for hiding this comment

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

nice and simple approach 👍🏻

Copy link
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

LGTM

@ControlCplusControlV ControlCplusControlV merged commit 2c46f4e into FuelLabs:master Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fuel-database
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fill unrechable! function in DummyDB that is used for tests
4 participants