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

Crop all print from Tests and Sources. #673

Merged

Conversation

yaroslavyaroslav
Copy link
Collaborator

Closes #639

@yaroslavyaroslav yaroslavyaroslav changed the title Crop all print from Tests. Crop all print from Tests and Sources. Nov 17, 2022
janndriessen
janndriessen previously approved these changes Nov 17, 2022
Copy link
Collaborator

@janndriessen janndriessen left a comment

Choose a reason for hiding this comment

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

LGTM! 👌 You might wanna wait with merging before the previous PRs are merged. Otherwise fun rebases ahead. ;D

@yaroslavyaroslav
Copy link
Collaborator Author

Yep, I'm not rushing with this one.

JeneaVranceanu
JeneaVranceanu previously approved these changes Nov 18, 2022
@JeneaVranceanu JeneaVranceanu dismissed stale reviews from janndriessen and themself via 68a75f5 November 18, 2022 11:26
@JeneaVranceanu
Copy link
Collaborator

@janndriessen Conflict was resolved. Re-approve required.

JeneaVranceanu
JeneaVranceanu previously approved these changes Nov 18, 2022
@@ -124,6 +124,6 @@
// let userAddress = EthereumAddress("0xe22b8979739D724343bd002F9f432F5990879901")!
// let tokenBalance = try await token.read("balanceOf", parameters: [userAddress] as [AnyObject])!.decodedData()
// guard let bal = tokenBalance["0"] as? BigUInt else {return XCTFail()}
// print(String(bal))
// )
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should drop this file.
Worth taking a look at what is useful inside and if we don't already have similar tests that weren't using promises.

Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu left a comment

Choose a reason for hiding this comment

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

The last thing before approval: please, go over all tests and replace the following ->

if case .notYetProcessed = receipt.status {
    return
}

Must be replaced with

if case .notYetProcessed = receipt.status {
    throw Web3Error.processingError(desc: "Transaction is not yet complete")
}

@yaroslavyaroslav
Copy link
Collaborator Author

@JeneaVranceanu sure, I'm no rushing with this one, since it'll be merger after @pharms-eth PRs, which I think would be merged within several weeks or so.

@yaroslavyaroslav yaroslavyaroslav merged commit 736dff6 into web3swift-team:develop Dec 14, 2022
@yaroslavyaroslav yaroslavyaroslav deleted the clean-tests-output branch December 14, 2022 13:07
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.

Remove all print output from tests
3 participants