scaffold-eth / se-2-challenges

SpeedRunEthereum challenges (Powered by Scaffold-ETH 2)
https://speedrunethereum.com
MIT License
91 stars 176 forks source link

Ch0: Update test for the autograder #137

Closed carletex closed 8 months ago

carletex commented 8 months ago

1

If doesn't affect the test workflow for the builder, we can merge it.

We need to do this for all the other challenges too. SE-1 tests did this too (e.g. https://github.com/scaffold-eth/scaffold-eth-challenges/blob/challenge-1-decentralized-staking/packages/hardhat/test/challenge_1.js), but not sure why/when we changed it in the SE-2.

rin-st commented 8 months ago

Probably it was changed because at the top of the file we have

//
// this script executes when you run 'yarn test'
//
// you can also test remote submissions like:
// CONTRACT_ADDRESS=0x43Ab1FCd430C1f20270C2470f857f7a006117bbb yarn test --network sepolia
//
// you can even run mint commands if the tests pass like:
// yarn test && echo "PASSED" || echo "FAILED"

and it works with myContract = await ethers.getContractAt("YourCollectible", contractAddress); but doesn't work with

// download- or not doesn't matter
contractArtifact = `contracts/download-${contractAddress}.sol:YourCollectible`;
const YourCollectible = await ethers.getContractFactory(contractArtifact);
myContract = await YourCollectible.deploy();

also noticed that for some reason network hardhat is hardcoded in test script of package.json "test": "REPORT_GAS=true hardhat test --network hardhat",, so I needed to remove network to run network sepolia

technophile-04 commented 8 months ago

and it works with myContract = await ethers.getContractAt("YourCollectible", contractAddress); but doesn't work with

Umm didn't get it properly, is that await ethers.getContractFactory(contractArtifact) is not working for you ? for me it runs correctly.

EDIT: Ohh I see, I think you were talking about running test on sepolia

"test": "REPORT_GAS=true hardhat test --network hardhat"

Yup we should remove this, btw I think in autograder we are not passing --network flag while running tests.

Btw found this gem if anyone wants to learn how autograder works internally not sure if its outdated though https://youtu.be/Rq0uW-lDngs?si=jM7GupcyDkQa6Mx-&t=1308


//
// this script executes when you run 'yarn test'
//
// you can also test remote submissions like:
// CONTRACT_ADDRESS=0x43Ab1FCd430C1f20270C2470f857f7a006117bbb yarn test --network sepolia
//
// you can even run mint commands if the tests pass like:
// yarn test && echo "PASSED" || echo "FAILED"

Btw I also think we should remove this, it might be misleading for the one who is following the challenge because if he run CONTRACT_ADDRESS=0x43Ab1FCd430C1f20270C2470f857f7a006117bbb yarn test --network sepolia it won't work at all since contracts/download-${contractAddress}.sol:YourCollectible is not be present.

Also it's not helpful for autograder too(since in autograder I think we are just running CONTRACT_ADDRESS=0x43Ab1FCd430C1f20270C2470f857f7a006117bbb yarn test (without --network flag)

But please feel to correct to me! I might be something here 😅

carletex commented 8 months ago

Thanks for the review Rinat & Shiv!

"test": "REPORT_GAS=true hardhat test --network hardhat",, so I needed to remove network to run network sepolia Yup we should remove this, btw I think in autograder we are not passing --network flag while running tests.

Yep, noticed that too! The autograder is currently passing the flag, but I was going to remove it since it was already hardcoded in the challenge. So maybe we want to remove it from the challenges (if the yarn test works by default on hardhat) and roll it back in the autograder PR??

https://github.com/austintgriffith/speedrun-grader/pull/9/files


OK, so if we want to keep the ability for people to test remote contracts we should have 3 options:

  1. Local test (for speedrunners)
  2. Remote test (for speedrunners)
  3. Fetch-contract-local-test (for te autograder)

I could tweak the autograder as needed (like declaring a new env var just for the autograder: e.g. DOWNLOADED_CONTRACT_ADDRESS)

What do you guys think?


Btw found this gem if anyone wants to learn how autograder works internally not sure if its outdated though https://youtu.be/Rq0uW-lDngs?si=jM7GupcyDkQa6Mx-&t=1308

Damn :sweat_smile: hahaha

technophile-04 commented 8 months ago

Actually we have hardcoded --network hardhat in main SE-2 repo too.

So if you don't pass flag it defaults to "defaultNetwork" set in hardhat.config.js.

So if we plan to remove the hardcoded -network hardhat which I am leaning too but lol not 100% sure since hardcoding it also kindof make sense (basically saving people from running test script on mainnet / testnet)

In case we plan to remove the hardcoded --network hardhat and this will also allow people to remote test contract

TODO:

1 . Specifically in challenge-0 README.md move yarn test above Deploying the contract section (Since in deploying contract section we are telling to update defaultNetwork: "sepolia")

In other PR 2 . Update base-tempalte-challenge and other challenges to remove hardcoded --network hardhat flag

3 . Update SE-2 main repo

carletex commented 8 months ago

Actually we have hardcoded --network hardhat in main SE-2 repo too. So if you don't pass flag it defaults to "defaultNetwork" set in hardhat.config.js.

Oh oh, this makes sense.

So:

image

IDK, this looks like a lot of hassle for allowing testing a live contract (which I don't feel is super useful.... I personally have never done it.)


The alternative is to just merge this PR (+ change the top comment on the test file), right? Now I'm leaning more toward this... haha

But still open to anything!

technophile-04 commented 8 months ago

The alternative is to just merge this PR (+ change the top comment on the test file), right? Now I'm leaning more toward this... haha

++++, I think maybe we should remove top comment since I feel its not worth it

https://github.com/scaffold-eth/scaffold-eth-challenges/blob/challenge-1-decentralized-staking/packages/hardhat/test/challenge_1.js

Btw, also even though in above file we are saying "Connect to address" we are actually not connecting and instead deploying a new contract check https://github.com/scaffold-eth/se-2-challenges/pull/137#discussion_r1499507364

I just tried testing the remote contract and it took (11m) :

Screenshot 2024-02-22 at 8 46 59 PM

This is will take alot more time for next challenges since they have more tests. So its not worth testing on remote contract.

So conclusion :

  1. Remove top comment from test and merge this PR

  2. Remove top comment and follow let contractArtifcat: string logic for other challenges 🙌

carletex commented 8 months ago

Updated the top comment!

The (updated) autograder seems to be working with this change:

image


Now we can create them for the other challenges.

It'd also be nice if some of you ran the autograder locally & test... so you get some «knowledge» on it.

Thank you!

rin-st commented 8 months ago

So conclusion :

  1. Remove top comment from test and merge this PR
  2. Remove top comment and follow let contractArtifcat: string logic for other challenges 🙌

Agree with you guys!

It'd also be nice if some of you ran the autograder locally & test... so you get some «knowledge» on it.

Thanks for the link to autograder, now I know how that magic works! I've read the code and tested it, and your changes working great for challenge 0 👍

great job!

carletex commented 8 months ago

Thank you both!!

We can also include it here: https://github.com/scaffold-eth/se-2-challenges/pull/136