azimuth icon indicating copy to clipboard operation
azimuth copied to clipboard

ERC721 compatibility tests don't pass

Open jtobin opened this issue 5 years ago • 8 comments

IIRC this isn't a concern (in that everything works, and we have reasons why the existing tests fail), but it's annoying. What's up with these, and can we fix them? Should we just delete the tests that we know fail?

Update (2020/07/23)

I've changed the title of this issue to reflect that it's only these specific tests that don't pass. They're no longer included in the main test suite, and can be found in test-extras instead.

jtobin avatar Apr 18 '19 15:04 jtobin

Wait huh, they don't? Last I checked only PlanetSale tests sporadically tripped over a tiny inaccuracy in account balance.

Fang- avatar Apr 18 '19 18:04 Fang-

The PlanetSale tests indeed still do that, but I also noticed some problems with NFTokenMock. I think I used to know what the latter was related to, but don't anymore.. 😄

Re: the PlanetSale tests, since we know about the small handful-of-wei inaccuracy and haven't judged it to be an issue, perhaps we should just relax the passing condition by comparing within +-20 wei or whatever is required (instead of a hard equality check).

jtobin avatar Apr 18 '19 21:04 jtobin

The NFTokenMock function should be consistent until you change the Ecliptic.sol code? iirc there was some web3-side weirdness with function overloading. If that's still a thing, it might be fixable by just using a more recent web3 version.

Fang- avatar Apr 18 '19 23:04 Fang-

Yeah I'll take a look at the NFTokenMock business. Will reopen in the meantime.

jtobin avatar Apr 18 '19 23:04 jtobin

I took a look into the NFTokenMock tests and they can just fail (apparently) nondeterministically. Sometimes they all pass, other times one or another fails due to an unhandled EVM revert.

It might be worth updating the truffle version, since there's been a new major release, and that will include a newer web3. I'll close this one in any case.

jtobin avatar Apr 30 '19 00:04 jtobin

(Reopening this because the NFTokenMock test problems have annoyed me yet again, so this is clearly still an issue..)

jtobin avatar Jun 19 '19 22:06 jtobin

#30 may have finally fixed this. I observed the test suite failing occasionally when I checked that branch pre-merge, but it's possible I simply forgot to npm install the new dependencies or something stupid like that. I've since run the test suite a bunch of times and haven't seen any of those weird NFTokenMock issues.

I'll be bold, close this, and hope I never have to open it again.

jtobin avatar Jul 21 '20 12:07 jtobin

^ nm

jtobin avatar Jul 21 '20 16:07 jtobin