Feature/uint enumerable set tests #2253

[julianmrodri]

#2253

Fixes #2253

Implemented the required tests for UintSet following the same approach taken for AddressSet, and implementing a common behavior.. Regarding the Mock contract I split the implementation in two separate contracts one for AddressSet and one for UintSet.

All linter checks and tests running correctly.

Files modified:

• test/utils/EnumerableSet.test.js
• test/utils/EnumerableSet.behavior.js (added)
• contracts/mocks/EnumerableSetMock.sol

[julianmrodri]

@nventuro @frangio This PR is ready to be reviewed. I noticed we were missing tests for EnumerableSet.UintSet so I added them.

I recommend looking at each commit individually in order to easily understand the changes, as they got mixed quite ugly by Github at the file level.

[nventuro]

Hm, it seems like I overlooked this when adding UintSet and the generic Set.

We could follow the approach taken in this PR, though it would increase complexity and number of tests in the suite. A way to do it might be to have a set behavior that receives three values, and then call it for each set type:

describe('EnumerableSet', function () {
  describe('AddressSet', function () {
    shouldBehaveLikeSet(accountA, accountB, accountC);
  });
  
  describe('UintSet', function () {
    shouldBehaveLikeSet(numberA, numberB, numberC);
  });
});

Alternatively we could just test the Set and assume that the specialized variants (which aren't more than casts) are implemented correctly. An issue with this is that Set's functions are private, so exposing those would be an issue. If we assume the casts are correct, then just testing any variant (like the current AddressSet tests) should suffice.

@frangio what do you think?

[julianmrodri]

Thanks @nventuro. I like the first proposal you mention. IMHO it is better to do that instead of just assuming the casts are correct. Even though they are basically casts, it is also quite a bunch of code that would remain uncovered (for UintSet for example )

[frangio]

I think it's desirable to test each typed variant separately. Using a behavior sounds like a good approach.

[nventuro]

Awesome! @julianmrodri do you think you can go ahead and implement the tests in the way described above?

[julianmrodri]

Sure! Will do. Thanks guys for your help.

…

On Mon, Jun 1, 2020 at 4:18 PM Nicolás Venturo ***@***.***> wrote: Awesome! @julianmrodri <https://github.com/julianmrodri> do you think you can go ahead and implement the tests in the way described above? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2254 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANNVGDUQAGXAQR5XFGYTGUDRUP5IVANCNFSM4NPG6UNQ> .

-- [image: IOVlabs - Building Value Networks] <https://www.iovlabs.org> Julian M. Rodriguez RIF Product Owner iovlabs.org <https://www.iovlabs.org> [image: RSK - Smart Contract Network] <https://www.rsk.co/> [image: RIF OS - Taking the Internet of Value one step forward] <https://www.rifos.org/>

[frangio]

Thank you for the awesome contributions!

[julianmrodri]

@nventuro @frangio This is ready to be Reviewed. It is implemented using behavior. Looks much better, thanks for this suggestion.

[nventuro]

This looks great @julianmrodri, thank you very much for your help!