Openzeppelin-contracts: Improve ERC721 tests

[nventuro]

The account names refactor (#1137) didn't cover ERC721 because the whole suite is somewhat problematic: there are multiple references to accounts with weird names (e.g. creator, sender, etc), inconsistent naming, among others. We should take a look at these four files and polish them a bit so that they are more in-line with the rest of the suite. While there's no testing guide yet (#1077), some test suites can be used as a reference point, like ERC20 and Escrow.

[elopio]

As mentioned in https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1256 by @nventuro , we should also have a specific test block for getApproved

[logeekal]

This seems to be an old issue. If it is still open I can pick this up. I am currently setting up the project, will report back with findings.

Edit:
@nventuro , Okay so i went through the source and test files and observed that tests are pretty exhaustive.
Could you please give introductory pointers on what needs to be done in terms of improving the test case?

In the related bug #1137 , you point our that ERC721 tests are mess. IF you can give more detail on what needs to be improved, it will be helpful.

[nventuro]

Hey @logeekal, thanks for volunteering for this!

I haven't looked at these tests in quite some time, but like you said, they _are_ correct, and have 100% line coverage. The issues that I recall with them come from a maintainability/readability point of view, as mentioned in one of the comments:

there are multiple references to accounts with weird names (e.g. creator, sender, etc), inconsistent naming, among others

That said, it is _very_ possible that I was being too harsh when evaluating these tests :sweat_smile: So don't sweat it if you don't find anything other than these naming issues I mentioned.

[logeekal]

Thanks @nventuro for your revert, I will get back to you after looking at the code again. Also as @elopio has pointed above about missing getApproved test, I will add that as well.

Cheers.

[frangio]

@logeekal Cool! Feel free to suggest improvements if you think of anything else. :slightly_smiling_face:

[logeekal]

@logeekal Cool! Feel free to suggest improvements if you think of anything else. 🙂

Sure @frangio . I am thinking of doing this fix in 2 parts.

1. I have added missing tests for getApproved function
2. I will go through the naming of the variables(creator, sender, etc) and will suggest some changes as per the original reason for opening the issue.

For the first one, I will be opening the pull request soon. You can let me know if it is not the correct way.

Thanks.

[logeekal]

@frangio & @nventuro ,
Regarding the naming consistency in ERC721 tests. I went through the complete test suite and below are my observations :

I did not see any code in ERC721 test suite using accounts[] format, so we are good on that front.

On the left most column, I give my suggestions but those are very minor changes, please let me know what you guys think and if I should go ahead with these changes.

[nventuro]

Niiice, I didn't expect a full-fledged spreadsheet :sweat_smile: Your suggestions look good, please go ahead!

[logeekal]

@nventuro , @frangio , Raised above pull request (#1891 ) for renaming variables. Please let me know if anything else is left on this bug.

[nventuro]

Closed via #1820 and #1891.