PatrickAlphaC / hardhat-nft-fcc

100 stars 139 forks source link

Modified and extended the unit tests for the RandomIpfs and DynamicSvg contracts #84

Closed pacelliv closed 1 year ago

pacelliv commented 1 year ago

A recap of the changes I made:

A. In the case of the dynamicSvg.test.js:

B. In the case of the randomIpfs.test.js:

PatrickAlphaC commented 1 year ago

Awesome work!!

To keep the code minimal here, I'm going to close the pull request, but doing this extra credit will hone your skills to get better at writing tests. Great work!

pacelliv commented 1 year ago

Thank you and I understand, I also thought I made a lot changes but it was worth trying.

I have a question, in the dynamicSvg test file there is a comment that says "Maybe some tokenURI oddities", I thought about that but I couldn't think of any. The only thing I test was switching the tokenURI based on the highValue paid by the minter calling the updateAnswer method from the Aggregator, but I don't think this counts as an oddity.

Could you give me ideas about a few oddities so can try to test them?

PatrickAlphaC commented 1 year ago

We could be more explicit with checking that the tokenURIs are being correctly formatted even with weird data or weird SVGs.

It's not crazy important tho. A fuzz test would be nice here (but... I didn't cover those in the course)