Separate zkSync Tests to Separate CI Step

[nlordell]

This PR moves the zkSync tests outside of the normal test NPM script. Since this would make it no longer run by default in CI, we added a separate tests-zk step to run the zkSync tests only.

This split allows us to potentially merge PRs when zkSync tests fail as long as the normal test suite does not. It is not the first time that CI starts failing without cause because of zkSync tests. As far as I can tell, the zkSync tooling downloads an in-memory node binary that it starts for actually executing the tests. In the past, tests have randomly started to fail because of:

• Changes to the behaviour of the in-memory node (example)
• Binary no longer hosted on the same URL (example)

When this happens, we do not want to block changes that are unrelated to contract logic (as they should not, in theory, have any affect on the zkSync tests).

Furthermore, running the zkSync tests takes quite a bit longer than the regular tests, so this also makes local npm test a bit faster.

[mmv08]

I'm slightly torn to be honest, because:

1. I agree with your assessment
2. At the same time, we could take these failures as a reminder to keep our tooling up to date (we don't have a process for this)

we do not want to block changes that are unrelated to contract logic (as they should not, in theory, have any affect on the zkSync tests).

but we cannot make zksync tests mandatory when PRs change the contract logic, right? so maybe we should continue being on the defensive side and update the tooling instead

[nlordell]

At the same time, we could take these failures as a reminder to keep our tooling up to date

I agree - which is why I moved the tests to a step (that will still show up as a failure and be a reminder for us) but not block development. I think from a CI perspective, having non-determinism in tests is a huge problem (there is a reason we have a lock file so all NPM dependencies and JS that we pull in is at an exact version).

I don't think we shouldn't fix the zkSync tests, but I also don't know how long it will take (zkSync testing has been challenging in the past), and it is blocking the merge of things we want to have for 1.5.0.

[mmv08]

i created a pr for dependencies updates #940

[nlordell]

I still think zkSync should be a separate CI step to be honest. We can configure it to be mandatory in the project settings (so it is effectively the same) but it makes things more flexible for us.

I also think the test run-times are a bit too high right now... We can discuss in our standup today.

[remedcu]

+1

[nlordell]

Just to follow up here, tests-zk is a required CI check to pass: