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

Revive tests #384

Merged
merged 34 commits into from
Aug 8, 2023
Merged

Revive tests #384

merged 34 commits into from
Aug 8, 2023

Conversation

glottologist
Copy link
Collaborator

No description provided.

@Laucans Laucans force-pushed the 253-revive-tests-1 branch 3 times, most recently from 2cbb29f to 265579e Compare July 27, 2023 14:55
@Laucans Laucans force-pushed the 253-revive-tests-1 branch 5 times, most recently from 3bf7212 to e891593 Compare July 27, 2023 17:03
Copy link
Contributor

@aguillon aguillon left a comment

Choose a reason for hiding this comment

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

Pretty cool!

I have some minor remarks here and there but otherwise the tests work. I'm not sure why they are so slow though? You don't seem to use time-controlling operations here, or did I miss something?

You also don't test deposits that much. Did you just choose to postpone this to another PR or is something blocking else blocking?

let add_swap_pair_should_fail_if_user_is_non_admin =
Breath.Model.case
"test add swap pair"
"should be fail if user is non admin"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"should be fail if user is non admin"
"should fail if user is non admin"

Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised by this but you're the native speaker after all (and you use this form in several places as well). You do use "should fail" in lots of places though, so I'm not sure if it's a typo or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy and paste laziness! Will fix

Comment on lines 2 to 5
(*
This function is ported from the breathlyser library due to it not being in the packeged version. Once the method is available in the package, the method can be replaced.
*)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will switch it out

Comment on lines +40 to +50
let originate_module
(type a b)
(level: level)
(name: string)
(contract: (a, b) module_contract)
(storage: b)
(quantity: tez) : (a, b) originated =
let typed_address, _, _ = Test.originate_module contract storage quantity in
let contract = Test.to_contract typed_address in
let address = Tezos.address contract in

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, will remove

let ntol = TestUtils.tolerance_to_nat tolerance in
{
swap = swap;
created_at = Tezos.get_now ();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you should use get_now when you create orders, as it can be sensitive to the execution context of the test. Maybe just forge timestamps and pass them around as argument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

None of the tests really rely on it so it is just an easy timestamp generator.

@glottologist
Copy link
Collaborator Author

Pretty cool!

I have some minor remarks here and there but otherwise the tests work. I'm not sure why they are so slow though? You don't seem to use time-controlling operations here, or did I miss something?

You also don't test deposits that much. Did you just choose to postpone this to another PR or is something blocking else blocking?

This was the test that was originally blocked by getting the oracle price. I am working on them now, plus some redemption tests.

Regarding the slowness. Are you using "make test" ?

@glottologist glottologist changed the title WIP - Revive tests Revive tests Aug 8, 2023
@glottologist glottologist merged commit f6cc409 into version-3 Aug 8, 2023
1 check passed
@glottologist glottologist deleted the 253-revive-tests-1 branch August 9, 2023 10:48
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.

3 participants