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

test: add integration tests for Upgradable #79

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Conversation

mooori
Copy link
Contributor

@mooori mooori commented Feb 9, 2023

Adds integration tests and removes the corresponding unit tests. The background and motivation are described in #51, which does the same for Pausable.

Expands test coverage compared to previous unit tests.

@mooori mooori added the testing label Feb 9, 2023
Comment on lines +382 to +383
/// TODO stage code that corresponds to a valid contract. Call a method of that contract to verify
/// the staged code was deployed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Carrying over this TODO. I suggest to tackle it in a separate PR.

@mooori mooori marked this pull request as ready for review February 9, 2023 10:23
@mooori mooori requested a review from birchmd February 9, 2023 10:24
Copy link

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

LGTM, nice work

Comment on lines +194 to +196
// Estimating a number of blocks to skip based on `duration` and calling `fast_forward` only
// once seems more efficient. However, that leads to jittery tests as `fast_forward` may _not_
// forward the block timestamp significantly.
Copy link

Choose a reason for hiding this comment

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

I feel like there's a middle ground here where we can fast-forward in larger steps until the duration to wait is smaller. For example, if we are skipping 10s into the future then it seems safe that we could skip 5 blocks and then check how much farther we need to go.

But then again, this is only used in tests so you're probably right that it is not worth the additional complexity for a minor speed-up. Let's leave it as-is unless it becomes a problem.

Copy link
Contributor Author

@mooori mooori Feb 9, 2023

Choose a reason for hiding this comment

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

It’s tricky because we need to move forward in time but worker.fast_forward moves forward in blocks. Since forwarding by multiple blocks still may not forward the timestamp, I guess finding that middle ground is difficult. That’s why I went for this very naive approach.

@mooori mooori merged commit 5ae3696 into master Feb 9, 2023
@mooori mooori deleted the upgradable-itests branch February 9, 2023 17:09
birchmd pushed a commit that referenced this pull request Feb 20, 2023
* Add integration tests

* Add Deserialize to derived traits

* Remove unit tests

* Remove dead util code for unit tests

All plugins are now tested in integration tests.

* Remove outdated paragraph from README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants