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.

Sign up for GitHub

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

Jump to bottom

Add unit test specific to Address utils (#1251) #1316

Merged
merged 1 commit into from
Sep 12, 2018
Merged

Add unit test specific to Address utils (#1251) #1316

merged 1 commit into from
Sep 12, 2018

Conversation

jbogacz
Copy link
Contributor

@jbogacz jbogacz commented Sep 11, 2018

🚀 Description

  • Added AddressImpl as contract proxy to invoke and test Address library
  • 'Is account address' verified on truffle 'creator' variable
  • 'Is contract address' verified on SimpleToken instance deployed in truffle testing context
  • [+] 📘 I've reviewed the OpenZeppelin Contributor Guidelines
  • [+] ✅ I've added tests where applicable to test my new functionality.
  • [+] 📖 I've made sure that my contracts are well-documented.
  • [+] 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

@jbogacz jbogacz force-pushed the fix/add-unit-test-to-address-utils-#1251 branch 2 times, most recently from 8e25139 to 2b1c8c6 Compare September 11, 2018 06:36
@jbogacz jbogacz changed the title Add unit test specific to Address utils (#1251) Add unit test specific to Address utils (#1251) #area:tests Sep 11, 2018
@jbogacz jbogacz changed the title Add unit test specific to Address utils (#1251) #area:tests Add unit test specific to Address utils (#1251) Sep 11, 2018
nventuro
nventuro reviewed Sep 11, 2018
View reviewed changes
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

This is a big one @jbogacz, thanks! Looks like this was only indirectly tested in the ERC721 tests.

contracts/mocks/AddressImpl.sol Outdated
@@ -0,0 +1,14 @@
pragma solidity ^0.4.24;

import "../../contracts/utils/Address.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace for import "../utils/Address.sol"; (see #1321)

contracts/mocks/AddressImpl.sol Outdated
{
return Address.isContract(account);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you add a newline at the end here? Not sure why the linter doesn't catch this

test/Address.test.js Outdated
require('chai')
.should();

contract('Address', function ([_, creator]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd rename creator to anyone, since we don't really need a creator here.

test/Address.test.js Outdated
});

it('should return false for account address', async function () {
const isContract = await this.mock.isContract(creator);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace this (and the other test) for (await this.mock.isContract(creator)).should.equal(false);? See #1216

test/Address.test.js Outdated
});

it('should return true for contract address', async function () {
const contract = await SimpleToken.new({ from: creator });
Copy link
Contributor

Choose a reason for hiding this comment

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

This from: creator isn't really needed, why is why I suggested the rename, and removing this bit (to avoid unnecessary confusion about a creator address) :)

@nventuro nventuro added kind:improvement tests Test suite and helpers. labels Sep 11, 2018
@nventuro nventuro added this to the v2.1 milestone Sep 11, 2018
@nventuro nventuro self-assigned this Sep 11, 2018
@jbogacz jbogacz force-pushed the fix/add-unit-test-to-address-utils-#1251 branch from 2b1c8c6 to 2ac55f8 Compare September 12, 2018 04:30
@jbogacz
Copy link
Contributor Author

jbogacz commented Sep 12, 2018

@nventuro be my guest :) I need some time to learn OZ flavors but I'm determined :) Anyway, I've fixed your review comments. I've squashed my commits and --force pushed to my branch - hope it's fine with OZ policy

@jbogacz jbogacz force-pushed the fix/add-unit-test-to-address-utils-#1251 branch from 2ac55f8 to e297281 Compare September 12, 2018 04:39
@jbogacz
Copy link
Contributor Author

jbogacz commented Sep 12, 2018

I'm not sure what happened why 'static test' failed with message 'No output has been received in the last 10m0s'. Does it mean that job maximum 10 minutes was reached and Travis killed it automatically? If so, could you @nventuro kick it again, please?

@jbogacz jbogacz force-pushed the fix/add-unit-test-to-address-utils-#1251 branch from e297281 to 9564234 Compare September 12, 2018 15:01
@nventuro
Copy link
Contributor

@jbogacz I've seen that happen in a couple suite runs, not sure if there's something wrong with our suite (e.g. a missing await) that cuases that - a rerun usually works.

Regarding reviews, it's usually better to simply add commits on top, since we squash before merging anyway, and it makes reviewing easier, but for small PRs like this one it's fine either way :)

nventuro
nventuro approved these changes Sep 12, 2018
View reviewed changes
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Great work @jbogacz, thanks a lot!

@nventuro nventuro merged commit 7825caa into OpenZeppelin:master Sep 12, 2018
@jbogacz
Copy link
Contributor Author

jbogacz commented Sep 12, 2018

hell yeah! thanks!

@nventuro nventuro mentioned this pull request Sep 13, 2018
Closed
come-maiz pushed a commit that referenced this pull request Oct 21, 2018
@jbogacz @come-maiz
Add unit test specific to Address utils (#1251) (#1316)
f8b0cec 
(cherry picked from commit 7825caa)
@come-maiz come-maiz modified the milestones: v2.1, v2.0 Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nventuro nventuro nventuro approved these changes

Assignees

@nventuro nventuro

Labels
tests Test suite and helpers.
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jbogacz @nventuro @come-maiz