test(core): e2e test cases core maciState/Poll

[baumstern]

This PR adds end-to-end test cases of the off-chain state machine, focusing on sanity checks.

Related to #792

[netlify]

✅ Deploy Preview for maci-typedoc ready!

Name	Link
🔨 Latest commit	f8d4ccb
🔍 Latest deploy log	https://app.netlify.com/sites/maci-typedoc/deploys/65a02b95c462cd00086a2cc8
😎 Deploy Preview	https://deploy-preview-910--maci-typedoc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[ctrlc03]

Thanks for this, it can be very useful to ensure MACI works correctly, though I have some thoughts:

Currently these are gearing towards being e2e tests for the MaciState/Poll classes, which is definitely useful, though I wonder if it would be best to focus on unit testing MaciState/Poll as we already have e2e/integration tests that touch on MACI from 0 to end (though what we have now as e2e and integration tests are not the best, and in general they do not "test" much as failures could be for a variety of reasons - we have an issue open to track this #869).

If you strongly feel to keep these as it is, then I would ask you to please rename everything appropriately, e.g. move to a e2e folder inside core tests, rename to not be processMessages specific (as you are relying on tally votes too).

Otherwise, we now have an open PR which refactors the processMessage related functions, making processMessage public and parameterised with custom errors. This way it is now much easier to do unit testing and actually test that the different results are because of the different conditions (which is what is missing in this PR as well as our e2e/integration tests suite and that we need to fix). - Also the TestHarness class can possibly be adapted to work on our integration tests

[ctrlc03]

this is nice and can be very useful, but please document it and move it to an util file!

[baumstern]

This is specifically designed for testing purposes and does not require exporting

[baumstern]

Besides this point, we can consider designing a high-level module similar to this, something like:

class MACIHighLevel {
  init();
  vote();
  tally();
}

However, this is beyond the scope of the current PR

[ctrlc03]

The TestHarness class is still without any comments/documentation

[ctrlc03]

so we can't re-use it for other tests? 😔

[baumstern]

actually, TestHarness has been liberated for the good of all tests in 687bc92

[ctrlc03]

🙏 thanks for that and for documenting it

[baumstern]

Thanks for this, it can be very useful to ensure MACI works correctly, though I have some thoughts:

Currently these are gearing towards being e2e tests for the MaciState/Poll classes, which is definitely useful, though I wonder if it would be best to focus on unit testing MaciState/Poll as we already have e2e/integration tests that touch on MACI from 0 to end (though what we have now as e2e and integration tests are not the best, and in general they do not "test" much as failures could be for a variety of reasons - we have an issue open to track this #869).

If you strongly feel to keep these as it is, then I would ask you to please rename everything appropriately, e.g. move to a e2e folder inside core tests, rename to not be processMessages specific (as you are relying on tally votes too).

Otherwise, we now have an open PR which refactors the processMessage related functions, making processMessage public and parameterised with custom errors. This way it is now much easier to do unit testing and actually test that the different results are because of the different conditions (which is what is missing in this PR as well as our e2e/integration tests suite and that we need to fix). - Also the TestHarness class can possibly be adapted to work on our integration tests

These are e2e tests at the maci-core package level. They don't cover inter-package level e2e testing, which would involve both onchain and zk components. The main objective here is to ensure the integrity of the maci-core package's module, with a specific focus on the behavior of processMessages(). This is something our other end-to-end/integration tests don't cover and is outside their scope.

As a side note, these tests are black-box in nature, which is why they rely on the results of tallyVote()

[baumstern]

Thanks for the feedback @ctrlc03 , I've changed file name and modified it to check for a specific throw condition. Please have another look

[baumstern]

Hey @ctrlc03, I need write access to push some updates. I've fixed the ESLint errors on the branch

[ctrlc03]

Hey @ctrlc03, I need write access to push some updates. I've fixed the ESLint errors on the branch

Sorry for taking this long to get back to you! Would you be happy for us to complete this: we would fix conflicts and push a commit to fix eslint errors. Otherwise you could open a PR from a fork? up to you and how you prefer to get this done 🙂

[baumstern]

Hey @ctrlc03, I need write access to push some updates. I've fixed the ESLint errors on the branch

Sorry for taking this long to get back to you! Would you be happy for us to complete this: we would fix conflicts and push a commit to fix eslint errors. Otherwise you could open a PR from a fork? up to you and how you prefer to get this done 🙂

Check out the latest fixes here, and you can go from there. I would like to avoid opening redundant PR and keep things tidy

[ctrlc03]

Hey @ctrlc03, I need write access to push some updates. I've fixed the ESLint errors on the branch

Sorry for taking this long to get back to you! Would you be happy for us to complete this: we would fix conflicts and push a commit to fix eslint errors. Otherwise you could open a PR from a fork? up to you and how you prefer to get this done 🙂

Check out the latest fixes here, and you can go from there. I would like to avoid opening redundant PR and keep things tidy

Updated, thanks for your contribution :) lmk if it looks good to you
Docs explaining different test suites in core: https://maci.pse.dev/docs/testing-in-details

[baumstern]

Hey @ctrlc03, I need write access to push some updates. I've fixed the ESLint errors on the branch

Sorry for taking this long to get back to you! Would you be happy for us to complete this: we would fix conflicts and push a commit to fix eslint errors. Otherwise you could open a PR from a fork? up to you and how you prefer to get this done 🙂

Check out the latest fixes here, and you can go from there. I would like to avoid opening redundant PR and keep things tidy

Updated, thanks for your contribution :) lmk if it looks good to you Docs explaining different test suites in core: https://maci.pse.dev/docs/testing-in-details

thanks, the change looks good overall. just left one small comment