test: add simple tests that fork Infura networks

[mikeseese]

This PR adds a handful of simple tests to test forking against non-ganache networks, specifically Infura. I have configured Travis to inject the INFURA_KEY environment variable as a secret. Unfortunately, this adds ~10 seconds to tests but I think it's a valuable addition.

The tests will be skipped if the environment variable isn't defined (by default for dev environments)

I can add a dotenv requirement to consume a non-committed .env file if you want? For now, I'm manually injecting the environment variable when I run INFURA_KEY=... npm run test

[coveralls]

Coverage increased (+0.001%) to 82.574% when pulling 828ab93 on forking/add-infura-forking-tests into 196fcce on develop.

[nicholasjpaterno]

Overall, LGTM. One minor nitpick and one genuine question about infura archival access.

[davidmurdoch]

@seesemichaelj What are your thoughts on mocking these infura network requests?

[mikeseese]

@davidmurdoch I feel like mocking them provides no additional value. The purpose of these tests are to do the actual non-mocked requests. None of the current tests do any real-world forking, and I believe it's important we do those tests. The costs are very minimal in CI and don't affect devs during development testing. I rather they are skipped than mocked because the mocked responses are easily covered in other ganache-forking-ganache tests.

[davidmurdoch]

Are you ultimately trying to test Infura's failure scenarios? Or is this just for regression testing?

What specific test issues are better solved using an external source rather than mocking it locally? My concern is that archive-state Infura-based tests makes our testing closed-source.

[mikeseese]

We have a use case where users use Infura as a forking endpoint (i.e. Teams and basically any other tutorial or workshop out there that talks about Ganache forking); this PR tests that explicit use case. It seems a bit silly and unproductive for us to solely rely on whether or not forking works in tests based on testing with our own provider. It's an integration test.

Later down the road, I may add performance based tests for TDD to increase the performance of forking off of Infura.

I could make these tests non-archive-state, but I don't believe non-archive-state really tests performance and it's likely to be added in a later PR. I think it's totally okay for us to have "you must use an archival node to make these tests work or they'll be skipped"; if a user forks our repo and wants to test they will be skipped as if they didn't exist. I don't think this makes our testing closed source; it makes some additional tests inaccessible without certain resources. I'm not making existing tests inaccessible or causing a regression. I'm covering our butts by making sure we test the most used use case of forking. If it ever becomes obsolete, they can be removed. However, I would consider these additional tests as value-added to the ganache-core testing suite for the time being.

I could also move all these tests to the marshmallow repo (an internal Truffle Suite project in case someone else stumbles on this comment), but I honestly believe they belong in ganache-core

[mikeseese]

Further, it's possible that there will be some issues that are quicker to use mainnet states for reproduction than it is to spend the time to figure out the root problem. For example, see the test Compound made to reproduce the issue in #571

[davidmurdoch]

I can't take the time to write out a full/thoughtful response at the moment, but in case you are going to work the #571 make sure you check the history of this delete method: #482 (comment) . I don't think the solution is to replace delete with put(0), this will likely just move the goalpost.

[mikeseese]

I don't think the solution is to replace delete with put(0), this will likely just move the goalpost.

Good, I'm also skeptical

[davidmurdoch]

Okay, so I'm 👍 with running these tests as part of CI, but I'll want the test directories reorganized so we have a clear separation between smoke tests*, like these, and our existing unit and integration tests.

I currently (manually 😅) run a bunch of other smoke tests before publishing, I can make these Infura tests part of that process. My goal has always been to fully automate these pre-publish smoke tests before deployment, it just hasn't happened yet.

Perhaps a decent solution is to divide tests within the test directory into two parts: smoke and local (or some other name if you can think of something better). A new npm script should be added to run these smoke tests, and this npm script should be configured to be run by CI (Travis, currently), as well as on prepublish. Any secrets that are used by these tests should be added to Travis (which you've already done, thanks!), and to GitHub Secrets as we'll be switching Soon:tm:.

---

In case my lack of rebuttal isn't satisfactory: if it isn't clear what a test is testing, the test itself becomes a maintenance liability. For every problem/bug/feature we should have a test to exercise it specifically. Committing tests without an understanding of the root of the problem may save some time now, but can cause headaches later. I'm not definitively for automated smoke tests, but for every issue brought to light by a smoke test, the solution should have an accompanying unit/integration test that targets the issue.

---

Thanks for diving in to these issues!

---

_{* a better term for these tests might be "system tests", but if they aren't testing a specific feature of the system, that might not be too accurate a term either... but I digress.}

[mikeseese]

In case my lack of rebuttal isn't satisfactory: if it isn't clear what a test is testing, the test itself becomes a maintenance liability. For every problem/bug/feature we should have a test to exercise it specifically.

I argue this tests a feature: being able to use the Infura service as a forking candidate

but I'll want the test directories reorganized so we have a clear separation between smoke tests, like these, and our existing unit and integration tests...Perhaps a decent solution is to divide tests within the test directory into two parts: smoke and local

Can you define exactly what you want me to do? Do you just want me to move test/* into test/local/* and move test/forking/infura to test/smoke/infura? Are there other current tests that are supposed to be moved? Ganache's testing is already poorly organized, and I'm not sure if I'm the right person to start determining what's an actual test and what's a smoke test.

This was supposed to be a simple value-added PR, and we've spent more time talking about the semantics of testing and organization than actually solving issues. If you don't want this PR, I'm totally fine with closing this PR and keeping these tests in my local branch to help me with my personal development usage of ganache. I just thought this was going to 1) help me solve forking bugs, 2) help future devs with forking bugs, and 3) provide a dead-simple set of tests that tests a fundamental feature of forking

[davidmurdoch]

I do appreciate the value-add of these specific tests, if that wasn't clear. I only want to develop a definitive separation of the types of tests developers and contributors must run. I try to avoid to temptation to pile on to the mess just because this is the way things are already.

Can you define exactly what you want me to do?

I was hoping you could align with me on these organization goals and further develop the testing methodology. If that's not something you are interested in, moving the directories as you've suggested is fine.

Are there other current tests that are supposed to be moved

There are no other smoke/system tests to move.

we've spent more time talking about the semantics of testing and organization than actually solving issue

I don't want you to become apathetic to ganache's development processes, or lack thereof; I highly appreciate your feedback and strongly value your expertise. If you do want to discuss further perhaps we should sync over Zoom (hit me up on Slack if you'd like!).

[mikeseese]

@davidmurdoch this is ready for review

[davidmurdoch]

Just need the GitHub secret added, i'm thinking of adding it to the Organization Secrets might make sense? Although since this is a ganache-specific key, maybe to the ganache-core secrets would be better. Your call!

Can you also update the Travis config to run both test suites? You'll probably need to add a new script config section to the config.

Thanks!

[mikeseese]

Alright @davidmurdoch this one is ready again