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

Port e2e tests to node #390

Merged
merged 3 commits into from
Mar 5, 2019
Merged

Port e2e tests to node #390

merged 3 commits into from
Mar 5, 2019

Conversation

kernelwhisperer
Copy link
Contributor

@kernelwhisperer kernelwhisperer commented Mar 2, 2019

Todo:

  • Fix CI

let stderr = ''

// return this function so the process can be killed
const exit = () => new Promise((resolve, reject) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same issue we had with bash, doing subprocess.kill() would not stop a background process.

For example:

13388 ~ 'node: npm test'
  `- 9164 ~ 'node: ava'
    `- 9488 ~ 'node: execa'   <- this is what I get in the test, which does not want to die :(
      `- 11808 ~ 'node: aragon ipfs' 
        `- 24548 ~ 'ipfs' 

Even if we listen to the 'exit' event in the CLI:

  process.on('exit', (code, signal) => {
    console.log('parent said exit', code, signal)
    ipfsProc.kill()
    process.exit()
  })

Most likely child.pid returns a Group ID, and node cannot kill groups on windows: nodejs/node#3617

Solution: https://www.npmjs.com/package/ps-tree which returns the 2 children which we can kill. 🔪

Copy link
Contributor

Choose a reason for hiding this comment

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

Great solution 🎉

})
}

test('should return the correct version', async t => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some issues with nixt:

  • for some reason the callback style .end() function did not play nice with ava (could have been because of me), had to extend it to support promises.
  • does not support snapshots - I don't think there is a way to return the stdout so we cannot use our own assertions.
  • does not support background tasks, which is why I created the startBackgroundProcess utility.
  • does not play nice with windows, commands need to be appended with .cmd, see issue, they should use execa or cross-spawn.

I think we should simply use execa and ava assertions instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a great tool anyway, but those are big downside in our case. Maybe when we test a bigger command like aragon run we will discover if only execa and ava is enough or we need at some point add other tooling

@kernelwhisperer kernelwhisperer requested a review from 0xGabi March 2, 2019 12:16
@kernelwhisperer
Copy link
Contributor Author

Finally made Travis work!

First it was failing because of the lockfiles: github:ahultgren/async-eventemitter and github:frozeman/WebSocket-Node

Then with ENOENT even though the packages should have been linked by lerna bootstrap (I even tried --force-local): https://travis-ci.org/aragon/aragon-cli/builds/501944557#L579

Thirdly, because of OS specific text, see src/util.js#normalizeOutput.

Ended up using hacky solution: running npm link after installing 🤔

@@ -3,14 +3,17 @@

# Dependencies
node_modules
*ipfs.cmd*
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 is created at the root folder when installing go-ipfs on windows 🤷‍♂️

- npm run lint
- npm run test
- npm run build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build script is automatically ran after install because of npm prepare

@kernelwhisperer kernelwhisperer force-pushed the feat/e2e-tests-in-node-v2 branch from 33242ad to ace33fa Compare March 5, 2019 14:44
Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Working great 🎉

@kernelwhisperer kernelwhisperer merged commit 4a63941 into master Mar 5, 2019
@kernelwhisperer kernelwhisperer deleted the feat/e2e-tests-in-node-v2 branch March 5, 2019 16:24
@kernelwhisperer kernelwhisperer added this to the aragonCLI v5.5 milestone Mar 5, 2019
@sohkai
Copy link
Contributor

sohkai commented Mar 6, 2019

On npm link, I don't know what the particular issue is, but apparently lerna bootstrap is dying for lerna link so maybe that's could be something to look into.

@kernelwhisperer
Copy link
Contributor Author

Then with ENOENT even though the packages should have been linked by lerna bootstrap (I even tried --force-local): travis-ci.org/aragon/aragon-cli/builds/501944557#L579

On npm link, I don't know what the particular issue is, but apparently lerna bootstrap is dying for lerna link so maybe that's could be something to look into.

Looks like it was an issue on my part actually 😅.
I described the issue here: https://github.com/aragon/aragon-cli/pull/439/files#diff-c3d8c26e209da78ecf8c501174996f65R18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants