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

Add integration tests. #2693

Closed
wants to merge 6 commits into from
Closed

Conversation

levino
Copy link
Contributor

@levino levino commented Apr 16, 2019

Description

I only added a simple integration test against a local ganache.

Fixes #2682

@nivida nivida added Enhancement Includes improvements or optimizations In Progress Currently being worked on labels Apr 16, 2019
@nivida
Copy link
Contributor

nivida commented Apr 16, 2019

We have currently a similar PR open here: #2688

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

Mine is much simpler. Not so simple any more.

@nivida
Copy link
Contributor

nivida commented Apr 16, 2019

Yep, just looked at the other and it looks way more complicated. We should probably merge this PR and use your structure as a base. @micahriggan @szerintedmi Would that be okay for both you?

@szerintedmi szerintedmi mentioned this pull request Apr 16, 2019
@levino levino force-pushed the feature/add-integration-tests branch from e2b912e to 0203645 Compare April 16, 2019 08:21
sources:
- ubuntu-toolchain-r-test
packages:
- g++-4.8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 4 space indent just follows eslint.config.

@szerintedmi
Copy link

Yep, just looked at the other and it looks way more complicated. We should probably merge this PR and use your structure as a base. @micahriggan @szerintedmi Would that be okay for both you?

I don't have a strong opinion, I'm just happy it's happening :) I need to try both locally to tell which approach would I prefer.

- nm21:/home/node/packages/web3-providers-ipc/node_modules
- nm22:/home/node/packages/web3-providers-ws/node_modules
- nm23:/home/node/packages/web3-shh/node_modules
- nm24:/home/node/packages/web3-utils/node_modules
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are necessary so that the npm install in the docker world does not write to the host and writes deps as root and also possibly for a different operating system.

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

To run locally it should suffice to:

npm install
npm run test:integration

Requirement: docker-compose installed on local machine.

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

The whole lerna crazyness is only slowing things down in this repo (e.g. one has to rollup all packages before one can run a test... WTF?). It should be undone asap.

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

Oh this part from the CI output is golden:

Installing to /home/travis/build/ethereum/web3.js/packages/web3-eth-iban/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-shh' in 9.6s:
> web3-shh@1.0.0-beta.52 dtslint /home/travis/build/ethereum/web3.js/packages/web3-shh
> dtslint types --onlyTestTsNext
Installing to /home/travis/build/ethereum/web3.js/packages/web3-shh/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-eth-ens' in 9.5s:
> web3-eth-ens@1.0.0-beta.52 dtslint /home/travis/build/ethereum/web3.js/packages/web3-eth-ens
> dtslint types --onlyTestTsNext
Installing to /home/travis/build/ethereum/web3.js/packages/web3-eth-ens/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-eth' in 9.8s:
> web3-eth@1.0.0-beta.52 dtslint /home/travis/build/ethereum/web3.js/packages/web3-eth
> dtslint types --onlyTestTsNext
Installing to /home/travis/build/ethereum/web3.js/packages/web3-eth/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-core-helpers' in 9.1s:
> web3-core-helpers@1.0.0-beta.52 dtslint /home/travis/build/ethereum/web3.js/packages/web3-core-helpers
> dtslint types --onlyTestTsNext
Installing to /home/travis/build/ethereum/web3.js/packages/web3-core-helpers/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-core-method' in 11.4s:
> web3-core-method@1.0.0-beta.52 dtslint /home/travis/build/ethereum/web3.js/packages/web3-core-method
> dtslint types --onlyTestTsNext
Installing to /home/travis/build/ethereum/web3.js/packages/web3-core-method/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-providers' in 22.3s:
> web3-providers@1.0.0-beta.52 dtslint /home/travis/build/ethereum/web3.js/packages/web3-providers
> dtslint types --onlyTestTsNext
Installing to /home/travis/build/ethereum/web3.js/packages/web3-providers/node_modules/dtslint/typescript-installs/next...
Installed!
lerna info run Ran npm script 'dtslint' in 'web3-core' in 15.3s:

@@ -52,6 +52,9 @@
"build": "rollup -c",
"dev": "rollup -c -w",
"test": "jest",
"docker": "docker-compose run --workdir='/home/node/packages/web3' web3 npm run",
"predocker": "docker-compose run web3 npm install --unsafe-perm",
"test:integration": "npm run docker test --testMatch='/**/*.Itest.js'",
Copy link

@szerintedmi szerintedmi Apr 16, 2019

Choose a reason for hiding this comment

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

do we need to introduce a new file extension? Can it be done with --scope flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I f****g hate jest and do not know what kind of magic it can do. It is very probable that there is a more elegant way.

Choose a reason for hiding this comment

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

tell me.. I spent 10 minutes writing the actual tests / docker stuff and hours in Lerna and Jest swamp </rant>

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

Why are linting, coverage and tests not run in parallel? There is no need to wait for linting to run the tests. Is TravisCI so bad?

@nivida
Copy link
Contributor

nivida commented Apr 16, 2019

The whole lerna crazyness is only slowing things down in this repo (e.g. one has to rollup all packages before one can run a test... WTF?). It should be undone asap.

Yes, it will be possible to remove lerna if we move the modules together to one web3 module.

Why are the tests not run in parallel? There is no need to wait for linting to run the tests. Is TravisCI so bad?

Didn't look at it closer. But yes we could parallelize it. Hopefully, GitHub will soon release the GitHub actions also for public repositories.

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

Boom, tests pass.

Missing whitespace.
@nivida nivida mentioned this pull request Apr 16, 2019
12 tasks
@coveralls
Copy link

coveralls commented Apr 16, 2019

Coverage Status

Coverage remained the same at 95.643% when pulling 2e44b74 on Levino:feature/add-integration-tests into 704be57 on ethereum:1.0.

@@ -52,6 +52,9 @@
"build": "rollup -c",
"dev": "rollup -c -w",
"test": "jest",
"docker": "docker-compose run --workdir='/home/node/packages/web3' web3 npm run",
"predocker": "docker-compose run web3 npm install --unsafe-perm",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will go into the docker container for service web3 and install all deps. Because I added a prepare step to the root package.json scripts after npm install lerna will bootstrap and also build all packages (all in the docker container / the mounted volumes). Then the integration tests will be run in the docker container.

The only problem I just realised is that the build output will be written to the host. One must also create volumes for all the /build folders. Damn lerna!

Copy link

@szerintedmi szerintedmi Apr 16, 2019

Choose a reason for hiding this comment

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

Ah, I think i got it, thanks. (I still getting lost in this spaghetti build process, I can't see the reason of all of this complication in the package)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not find it so spaghetti to be honest. I mean lerna is annoying and makes no sense here but other than that is usualy that one npm script triggers the other...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made virtual /dist folders for the docker container.

@nivida
Copy link
Contributor

nivida commented Apr 16, 2019

@levino I would rename the folder integrationTests to e2e.

@szerintedmi
Copy link

szerintedmi commented Apr 16, 2019

To run locally it should suffice to:

npm install
npm run test:integration

Requirement: docker-compose installed on local machine.

it hangs for me on this (after successful npm install, pulled PR branch just now)

$ npm run test:integration

> web3@1.0.0-beta.52 test:integration /Users/petro/ether/web3.js
> lerna run test:integration

lerna notice cli v3.13.1
lerna info Executing command in 1 package: "npm run test:integration"

@micahriggan
Copy link
Contributor

Yep, just looked at the other and it looks way more complicated. We should probably merge this PR and use your structure as a base. @micahriggan @szerintedmi Would that be okay for both you?

That's fine with me, mine was based off of the wrong branch anyway.

There are some aspects to mine that I think are important, and are missing here

  • Testing websockets
  • Testing geth and parity

Other than that, nice work, glad this is happening

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

To run locally it should suffice to:

npm install
npm run test:integration

Requirement: docker-compose installed on local machine.

it hangs for me on this (after successful npm install, pulled PR branch just now)

$ npm run test:integration

> web3@1.0.0-beta.52 test:integration /Users/petro/ether/web3.js
> lerna run test:integration

lerna notice cli v3.13.1
lerna info Executing command in 1 package: "npm run test:integration"

It just takes very long. You can go to the package (cd package/web3) and then run the tests there npm run test:integration then you can see the progress.

@levino
Copy link
Contributor Author

levino commented Apr 16, 2019

  • Testing websockets
  • Testing geth and parity

Please be more specific.

Websockets should be testable against ganache, right? Can geth and parity be put into any state? Especially empty state? Or do you want to run integration tests against Kovan etc? I think that is a very bad idea, because of the statefulness.

@micahriggan
Copy link
Contributor

micahriggan commented Apr 16, 2019

Parity and Geth both have a dev mode, which start empty chains, I was attempting to utilize that mode in my MR.

As for the websockets on ganache, definitely, those should be tested as well

But I think it'd be smart to have the tests run against each node, for each protocol. So that way you know if there's a breaking change that affects one node, or if there's some missing functionality.

I think what you have is good to merge for a starting point. The other nodes and protocol stuff can be done later

@levino
Copy link
Contributor Author

levino commented Apr 25, 2019

Remarks should have been adressed. Anything else?

{},
{},
allEventsLogDecoderMock,
abiModelMock
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes came from npm run travis.

const mac = keccak256(Buffer.concat([derivedKey.slice(16, 32), Buffer.from(ciphertext, 'hex')])).replace(
'0x',
''
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes came from npm run travis.

@levino levino force-pushed the feature/add-integration-tests branch from 8b9b28f to 2e44b74 Compare April 25, 2019 06:23
@levino
Copy link
Contributor Author

levino commented Apr 25, 2019

The build fails because the type defs do not pass linting for TsNext any more. I think that setup is badly configured. Branch 1.0 should also not build atm.

@levino
Copy link
Contributor Author

levino commented Jun 11, 2019

I can make this much simpler. I would only do it if there is interest in actually merging this. Is there @nivida ?

@nivida nivida closed this Jun 12, 2019
@nivida
Copy link
Contributor

nivida commented Jun 12, 2019

Thanks for asking @levino. I've renamed all the branches for having a better overview and to prepare the coming stable release. I will use this PR as base for the integration tests in the 2.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Includes improvements or optimizations In Progress Currently being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants