Integration tests

[nivida]

Description

The Web3.js library QA should get improved with integration tests. The goal is to test it against several node types (Geth, Parity, and Ganache).
These integration tests should run in a docker image for later adding it to the GitHub actions.

Expected behavior

I can execute npm run e2e and it will test the whole library with several node types.

Actual behavior

There are no integration tests.

[szerintedmi]

I'm more than happy to chip in with a few basic ganache tests.

Where these test should sit in the repo structure?

I would just launch a container with a given version of ganache for now - later travis matrix or whatever magic can be added to test against beta etc. versions.

Some mock contracts might be required to test methods , events etc. To keep things simple I would just add those contracts via some pre-test scripts. Later we can consider creating a web3js test container derived from the standard ganache image, already preloaded with those.

I would launch ganache with a deterministic mnemonic so that it's easier to test signing functions.

@micahriggan : AFAIK you started to work on this. where are you with it?

[nivida]

@szerintedmi Sounds great! But I would probably use Geth for now because it's the most used node from the EF and is more up to date with the JSON-RPC spec.

[szerintedmi]

I didn't suggest not to test against Geth. I meant I'm happy to contribute with ganache integration tests because that's our current pain point with web3.
Although the same tests should run against geth/ganache/parity , only the container would be different so it shouldn't be a big deal to cover them.

[princesinha19]

Yes, @nivida is correct. Ganache doesn't support many RPC methods which are supported by Geth and Parity. So, for now, we should consider Geth.
For eg. trufflesuite/ganache#408, trufflesuite/ganache#405 etc.

I know ganache is used by most of the developers as a TestRPC. I also use ganache, but it's not up to date.

[szerintedmi]

Again, the same test should run against Geth/Parity/Ganache. So it's only a question which docker image you are running your tests against. Geth is not so important for us atm but it's literally should be one command to launch a parity or geth container. The heavy lifting here is to create proper tests.

Anyway, if ganache is not supporting something then integration tests are a great to expose it. Also running it against future ganache releases will show if they implemented it as you expected.

We use ganache for our on-chain tests. It used to have a lot issues but they iterated a lot and it became very stable and fast.

(Btw I think trufflesuite/ganache#408 has been fixed. I commented there. Great example which would have been obvious with integration tests )

[sshelton76]

What about modifying this to test against any and all supplied RPC endpoints? Then we only need the one container + one parameter, maybe two parameters if there is login info required. It's going to be easier to maintain and those who want ganache or other older tech can see where those projects are falling behind, plus they can test vs services such as AWS blockchain in box and even Infura.

e.g.

npm run e2e https://rinkeby.infura.io/$ID

[szerintedmi]

npm run e2e https://rinkeby.infura.io/$ID

@sshelton76 : I think it's a great idea. Wondering if one or two param will be enough as each provider might require some options to run . but I guess we will see.

Would be great to test against Infura too. Do you have a thought how could we test transactions there? Do they have any mock or we would need to deploy mock contracts to a test network? That sounds a bit complicated for me...

I would propose one step a time and iterate. For me ganache tests are the easiest to start with as I have the most experience with that. Maybe I draft a very rudimentary first pass PR then we can discuss that?

(although first I have to make it work locally, npm test fails. looking into it but let me know if you aware of anything obvious I might do incorrectly)
UPDATE: test are running now, I just missed npm build :)
btw, npm install is not listed in the README, might be obvious but I thought npm bootstrap script does it.

FAIL tests/src/IbanTest.js
  ● Test suite failed to run

    Cannot find module 'web3-utils' from 'IbanTest.js'

      4 | 
      5 | // Mocks
    > 6 | jest.mock('web3-utils');
        |      ^
      7 | 
      8 | /**
      9 |  * Iban test

      at Resolver.resolveModule (../../node_modules/jest-resolve/build/index.js:229:17)
      at Object.mock (tests/src/IbanTest.js:6:6)

[levino]

Too general, not actionable.

[szerintedmi]

Too general, not actionable.

Indeed, it's an epic. I'm drafting a rudimentary PR as a conversation starter about approach.
Will push WIP soon, just trying to get out from Lerna and Jest rabbit holes.

UPDATE: great syncing of effort - just seen @micahriggan 's #2688 :))

[szerintedmi]

Lol, 3 of us :)
I won't push my PR, it doesn't add much to #2688 and #2693 and those are more than enough to start discussion.
I will add bits when you decided which PR to carry on with

[szerintedmi]

@nivida : I think you are pulling the trigger too soon by closing #2688 . It has been opened 7 hours ago. You are not leaving time for community review.

I'm still reviewing both PR-s and I wan't to try both locally too. And it's not about me, there are much more experienced people around who can't jump on immediately.

[nivida]

@szerintedmi Yeah, that's true but you were commenting there I should decide which PR should be the base. The decision was that I will take the PR from @levino. I think a simple base can be merged quickly and all further e2e tests have to be reviewed closer and from more than just one developer.

[szerintedmi]

@levino & @micahriggan :
Would you please confirm if I understand it well?

#2693

• maps the web3 package folders as docker volumes and not adding them to the container
• The web3 build is initiated and the test started from outside the container (scripts in package.json)
• Integration tests sitting across individual packages

#2688

• test_runner container ADD-s web3 packages and not mapping as volumes them
• container build scripts and test starting scripts are within container
• Integration tests are sitting in a separate folder

Both approaches has pros/cons which we can discuss just want to be sure I understand it correctly.

UPDATED based on @levino 's clarifications

[levino]

You are wrong about #2693. https://github.com/ethereum/web3.js/pull/2693/files#r275717827

I find it a bad idea to discuss implementation details in issues. Please let us continue this conversation in the PR.

[szerintedmi]

Thanks for explaining. Clarified. Is it correct now?

Anyway my 2 cents :

• I prefer build scripts called from the container.

  • More readable what's going on: I don't have to understand the complex build process of web3 in details. I read the dockercompose.yml and the Dockerfile and I get what's going on.
  • it's more contained - it doesn't entangles itself to the build process of the web3 package just do standard web3 install scripts
  • initiating the build from the main package.json adding more complexity to the already complex build process

• Usually I prefer unit tests next to the code but in this case, for an integration test it's better having them separate:

  • I don't have to navigate back and forth between paths like web3.js/packages/web-eth/tests/src/methods with an overwhelming number of other files and folders.
  • more maintainable : one who might contribute doesn't necessarily want to understand web3 internal implementation details.
  • it's a single place documenting the expected behaviour (could be even linked from docs as examples )

• ADD vs. volumes in Docker: I'm not familiar enough with the volumes method - will you be able to run that image without having the web3 repo cloned?

These can be addressed in either PR it's just the question which one is easier to start from.

I agree that implementation details shouldn't be here but some of it is generic approach question. Discussing these under your PR would result this thread buried if that's not merged. I'm OK to continue there if the decision was made to carry on with that PR.

Note: i'm not familiar with this codebase so it's just based on my initial glimpse into it.

[levino]

Should we create an issue for 1.2.x too? Why no integration tests there?

[cgewecke]

These are in.