Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitting tests #490

Merged
merged 27 commits into from
Feb 19, 2022
Merged

Splitting tests #490

merged 27 commits into from
Feb 19, 2022

Conversation

signorecello
Copy link
Contributor

So we needed e2e tests to be splitted as per #413 . I went ahead and made some more changes to improve them and make them easier to maintain on the long run. Here are some considerations to sustain this:

Organization stuff

  • I added a no-await-in-loop rule to eslint.rc, as we had about 70+ inline eslint rules exceptions. We can think of having them on a per-file basis though. As you know, we use await in loops to avoid nonce issues, mostly.
  • I separated the e2e tests into e2e-protocol and e2e-tokens, don't mind the naming though. The idea is to have tests related to challengers, proposers, chain-reorgs, future PoS tests, whatever is protocol-related, inside this folder. The other one would be to test anything ERC-related.
  • For each folder there's a index.test.mjs file that imports the tests from that folder. This allows us to skip specific files for development.
  • I personally like tests to not have a lot of info logs, so it's easier to read the results without a lot of messages on the console. So I added a VERBOSE env variable you can pass to tests, to see all the info logs (ex. VERBOSE=true npm run test-e2e-tokens). Otherwise, only error or other very relevant messages show on the console.
  • As discussed previously, I removed some unused tests like chain-reorg, etc.

Big changes in tests

  • We were doing some magic number stuff in tests that would break often, for example regarding transactionsPerBlock. I wanted tests to work with any transactionsPerBlock count by just setting an env variable. Also, tests would break when re-running them if there were any unprocessed transactions in the optimist mempool, as the L2 count would mismatch with the smart contract count, and transactions would fail. So I implemented a function to get the unprocessed transaction count, and another one that would attempt to empty the L2 mempool after every describe.
  • We were using a constants.mjs file with a bunch of key-values, which was cumbersome to read. I created some json files to save them nice and tidy and easy to maintain.
  • Web3 changes. So we had a bunch of things like submitTransaction, getBalance, closeWeb3Connection, etc that we would pass an instance of web3 to. I though it would make sense to make a class to abstract all the things we want with web3. So at the start of the tests we just get an instance of this class and use it without having to juggle with an web3 instance being passed around on every function call. It's also easy to clean up after each test. There's a getter .getWeb3() that will return you the web3 provider if you want to use the "raw" web3 provider.
  • I don't like logCounts as it looks a bit naive for the uninitiated. I tried to remove it and I failed miserably, we really need it. As per the point above, I created a subcribeTo method on the new Web3 class that hopefully abstracts the queueing of the logs. So logCounts is now logs and is an object with some arrays of logs inside, we just check the length if we actually need to count them. This way we can reuse the subscribeLogs method for every kind of blockchain event, newBlockHeaders, transactionSubmitted, newCurrentProposer, you name it. Easier to add some more events in the future.

Some other weird changes

  • As discussed previously, getLayer2Balances wasn't really working. I fixed that, and got rid of filterByCompressedPkd parameter as it wasn't being used anywhere. getLayer2Balances retrieves the nf3 instance's balance, and that's all. For the same reason I got rid of /balances-details, as we don't use it at all.

@signorecello
Copy link
Contributor Author

After talking with @druiz0992 I removed the wallet-test as it was failing inexplicably and the tests make little sense now given that we will have a brand new shiny wallet

@daveroga
Copy link
Contributor

We have to think about all config files related with tests. They were unified in the config module in the issue #398 about rationalizing configs.

@signorecello
Copy link
Contributor Author

We have to think about all config files related with tests. They were unified in the config module in the issue #398 about rationalizing configs.

Hey, add me as a reviewer on that one, let's merge it as it's been here for a while. I'll solve the conflicts with this one. I'm having a hard time making the tests pass reliably anyway.

Copy link
Contributor

@Westlad Westlad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice code! Just a few minor comments.

cli/lib/nf3.mjs Outdated Show resolved Hide resolved
cli/lib/nf3.mjs Show resolved Hide resolved
nightfall-client/src/app.mjs Show resolved Hide resolved
Copy link
Contributor

@ChaitanyaKonda ChaitanyaKonda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor makes the test files more modular and clear to understand.
This looks very clean and much easier to debug.

I requested a few changes as suggested in earlier comments.

@Westlad Westlad removed the question label Feb 17, 2022
@ChaitanyaKonda
Copy link
Contributor

Fixes #504

@signorecello signorecello merged commit 0a7c966 into master Feb 19, 2022
@signorecello signorecello deleted the zepedro/e2e-tests-split branch February 19, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants