Delete redundant tests #621
Conversation
| it("should add contract deployer as owner", async function () { | ||
| const contractOwnerAddr: string = await contract.owner() | ||
| expect(owner.address).to.equal(contractOwnerAddr) | ||
| }); |
There was a problem hiding this comment.
Testing the OpenZeppelin modifier, nothing to do with precompiles or our code.
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }); |
There was a problem hiding this comment.
addDeployer isn't a contract function, so this test really isn't testing anything. The allowlist logic is already tested.
There was a problem hiding this comment.
So, bit confused here. It seems like we define addDeployer in tasks.ts. The definition of addDeployer is as follows and does test the allow list logic here. Correct me if I'm wrong.
task("deployerAllowList:addDeployer", "Adds the deployer on the allow list")
.addParam("address", "the address you want to add as a deployer")
.setAction(async (args, hre) => {
const allowList = await hre.ethers.getContractAt("IAllowList", CONTRACT_ALLOW_LIST_ADDRESS)
// ADD CODE BELOW
await allowList.setEnabled(args.address)
await getRole(allowList, args.address)
})There was a problem hiding this comment.
I have no idea what those tasks are. But they aren't used when the code is run for the e2e tests. All of those addDeployer methods fail with some kind of method does not exist error.
There was a problem hiding this comment.
https://hardhat.org/hardhat-runner/docs/advanced/create-task Read more about tasks here. Seems to be used by hardhat as a cli command. These should be removed 👍
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }); |
There was a problem hiding this comment.
Another addDeployer
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }); |
There was a problem hiding this comment.
Another addDeployer
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }); |
There was a problem hiding this comment.
This is testing logic in the example contract, not the precompile. There's nothing in the precompile that prevents an admin from "revoking" itself.
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }); |
There was a problem hiding this comment.
Same comment as above.
| it("should add contract deployer as owner", async function () { | ||
| const contractOwnerAddr: string = await contract.owner() | ||
| expect(owner.address).to.equal(contractOwnerAddr) | ||
| }); |
There was a problem hiding this comment.
Same thing, no need to test OpenZeppelin logic.
richardpringle
force-pushed
the
delete-redundant-tests
branch
from
9026282
to
d5608c1
Compare
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }) |
There was a problem hiding this comment.
addDeloyer isn't a function.
There was a problem hiding this comment.
hmmm so these were failing but with wrong errors. totally bad testing...
good catch 👍
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }) |
There was a problem hiding this comment.
allowed can't revoke anyone including itself. Blocking a non-admin is already tested.
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }) |
There was a problem hiding this comment.
This tests a require statement in the example contract. The precompile doesn't have this functionality and is thus testing the EVM bytecode generated by the solidity compiler instead of testing the precompile(s).
| it("signer1 should be appointed as reward address", async function () { | ||
| let tx = await contract.setRewardAddress(signer1.address); | ||
| await tx.wait() | ||
| let rewardAddress = await contract.currentRewardAddress(); | ||
| expect(rewardAddress).to.be.equal(signer1.address) | ||
| }); | ||
|
|
||
| it("signer1 should be able to receive fees", async function () { | ||
| let previousBalance = await ethers.provider.getBalance(signer1.address) | ||
|
|
||
| // Send a transaction to mine a new block | ||
| const tx = await owner.sendTransaction({ | ||
| to: signer2.address, | ||
| value: ethers.utils.parseEther("0.0001") | ||
| }) | ||
| await tx.wait() | ||
|
|
||
| let balance = await ethers.provider.getBalance(signer1.address) | ||
| expect(balance.gt(previousBalance)).to.be.true | ||
| }) |
There was a problem hiding this comment.
why have we removed those? these are testing the actual logic in reward manager precompile.
There was a problem hiding this comment.
Oh weird, I swear I left a comment here before.
My assumption here was that the test was redundant as I thought that balance was stored the same way for contracts and EOAs. But after a little digging, looks like I'm wrong. Will fix!
There was a problem hiding this comment.
Ah, after looking again at the new DS-Tests, the logic is exactly the same. I don't think these tests are necessary. More granular testing unit/integration testing should happen for the Go code, but I think testing that "an address" is set as the reward-address and that the same address has an increased balance after a transaction single transaction is mined is sufficient.
There was a problem hiding this comment.
ah ok that was a redundant test. Some tests were put to clear possible confusions like "reward address should be same address as the wrapping contract" etc. so specifically put that one to show EOAs & contracts can both be reward address. but thats a reduntant test indeed.
| return | ||
| } | ||
| expect.fail("should have errored") | ||
| }) |
There was a problem hiding this comment.
hmmm so these were failing but with wrong errors. totally bad testing...
good catch 👍
richardpringle
force-pushed
the
delete-redundant-tests
branch
from
d5608c1
to
b2dece9
Compare
Why this should be merged
Will make changes in #601 easier to read/reveiw
How this works
Just deleting tests that are redundant or failing
How this was tested
N/A
How is this documented
N/A