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

Refactor tests #595

Merged
merged 13 commits into from
Apr 20, 2023
Merged

Refactor tests #595

merged 13 commits into from
Apr 20, 2023

Conversation

zajck
Copy link
Member

@zajck zajck commented Mar 30, 2023

Closes #593

I made three changes:

  • added a new util function setupTestEnvironment which deploys protocol. It can be reused across most of the tests. There are some exceptions (ConfigHandlerTest, ProtocolDiamondTest and ProtocolInitializationHandlerTest) where tests were written in a way that using setupTestEnvironment did not make sense
  • First beforeEach in most of the test files was changed into before block, where we take evm snapshot at the end of the deployment and then after each test call evm_revert. This significantly improved execution time. To take snapshot and revert back to it I added two more util functions (getSnapshot, revertToSnapshot)
  • I changed algorithm in split-unit-tests-into-chunks to make more even chunks which should on average result in lower execution time.

@zajck zajck added the Refactor label Mar 30, 2023
@zajck zajck self-assigned this Mar 30, 2023
@zajck zajck requested review from anajuliabit and mischat March 30, 2023 16:20
@zajck zajck mentioned this pull request Apr 3, 2023
Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

I was looking forward to this PR. Thank you!
just some small requests

scripts/util/split-unit-tests-into-chunks.js Outdated Show resolved Hide resolved
scripts/util/split-unit-tests-into-chunks.js Show resolved Hide resolved
test/util/mock.js Show resolved Hide resolved
@zajck zajck added the v2.2.0 label Apr 5, 2023
anajuliabit
anajuliabit previously approved these changes Apr 5, 2023
mischat
mischat previously approved these changes Apr 20, 2023
Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

not a big deal but I added a suggestion

@@ -284,8 +289,134 @@ function objectToArray(input) {
return result;
}

async function setupTestEnvironment(
contracts,
{ returnClient = false, returnAccessController = false, bosonTokenAddress, forwarderAddress } = {}
Copy link
Contributor

@anajuliabit anajuliabit Apr 20, 2023

Choose a reason for hiding this comment

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

Properties returnAccessController and returnClient from setupTestEnvironment args seem unnecessary to me. Why don't you return the entire extraReturnValues by default and leave the decision of which properties to extract from extraReturnValues directly in the test files? Since there are no "heavy" behaviors inside the ifs statements (such as promises, etc) it seems unnecessary to have these variables.

Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

sorry

test/util/utils.js Outdated Show resolved Hide resolved
@zajck zajck requested a review from mischat April 20, 2023 16:50
@zajck zajck requested a review from anajuliabit April 20, 2023 16:50
Copy link
Contributor

@anajuliabit anajuliabit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mischat mischat left a comment

Choose a reason for hiding this comment

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

:shipit:

@zajck zajck merged commit b5a58d9 into main Apr 20, 2023
@mischat mischat deleted the refactor-tests branch April 20, 2023 16:54
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.

Refactor and optimize tests
3 participants