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

test: measure codecov, add missing tests #359

Merged
merged 12 commits into from Mar 19, 2022

Conversation

antongolub
Copy link
Contributor

  • Tests pass

@antongolub antongolub force-pushed the test-refactoring branch 2 times, most recently from f249493 to e0d33bc Compare March 18, 2022 20:00
@antongolub
Copy link
Contributor Author

antongolub commented Mar 18, 2022

@antonmedv,

I'm stuck. How to make this work on ubuntu?

if (test('question')) {
  let p = question('foo or bar? ', {choices: ['foo', 'bar']})

  setTimeout(() => {
    process.stdin.emit('data', 'fo')
    process.stdin.emit('data', '\t')
    process.stdin.emit('data', '\r\n')
  }, 100)

  assert.equal((await p).toString(), 'foo')
}

@antongolub antongolub changed the title test: measure codecov, add missed tests test: measure codecov, add missing tests Mar 18, 2022
@antongolub
Copy link
Contributor Author

And we also need a bit less interactive script to check https flow:

if (test('Eval script from https ref')) {
  let p = $`node zx.mjs https://medv.io/example-script.mjs` // Need another script
  setTimeout(() => p.kill(), 500)
}

just echo would be nice

@antonmedv
Copy link
Collaborator

What about using zx to test zx?

let script = `
echo(await question('foo or bar? ', {choices: ['foo', 'bar']}))
`
await $`node zx.mjs ${script}`

Same with server:

$`nc -l 8080`
await $`curl :8080`

@antongolub
Copy link
Contributor Author

antongolub commented Mar 19, 2022

@antonmedv,
Got a trouble with nc, so the test hangs in an infinite loop. Kill the task plz

@antonmedv
Copy link
Collaborator

Killed.


if (test('Eval script from https ref')) {
let script = path.resolve('test/fixtures/echo.http')
let server = quiet($`while true; do cat ${script} | nc -l 8080; done`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With timeout? Problem we can’t merge them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing Content-Length header. Fixed.

let script = await question('foo or bar? ', {choices: ['foo', 'bar']})
$node zx.mjs <<< ${script}

foo or bar? file:///Users/antongolub/projects/zx/src/index.mjs:99
    let output = new ProcessOutput({
                 ^
ProcessOutput [Error]: 
  at file:///Users/antongolub/projects/zx/test/index.test.mjs:123:4
  exit code: 13
  at ChildProcess.<anonymous> (file:///Users/antongolub/projects/zx/src/index.mjs:99:20)
  at ChildProcess.emit (node:events:520:28)
  at maybeClose (node:internal/child_process:1092:16)
  at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)

Still raises err code 13. Seems like a deadlock detection https://stackoverflow.com/questions/46914025/node-exits-without-error-and-doesnt-await-promise-event-callback/46916601#46916601

@antongolub
Copy link
Contributor Author

antongolub commented Mar 19, 2022

Good news.

image

Wow, interesting: Nodejs v17 resolves localhost as IPv6 by default. nodejs/node#40537

@antongolub
Copy link
Contributor Author

antongolub commented Mar 19, 2022

@antonmedv

One more nodejs v17 task is still hanging. Kill again plz. The rest are fine.

@antonmedv
Copy link
Collaborator

Killed.

@antongolub
Copy link
Contributor Author

Any objections/suggestions on this PR? I think 99% code cov is a good point to start refactoring of zx inners.

@antonmedv
Copy link
Collaborator

Awesome work! Such test coverage is impressive!

@antonmedv
Copy link
Collaborator

Another thing for future improvements: make tests less dependent on each other and on common fixtures.

@antonmedv antonmedv merged commit a203460 into google:main Mar 19, 2022
@antonmedv
Copy link
Collaborator

Will be cool to add cov report as comment to PRs.

@antonmedv
Copy link
Collaborator

Looks like c8 is missing from deps.

@antongolub
Copy link
Contributor Author

I'm planning to decompose both tests and index.mjs inners. Thus they will become more atomic, loosely coupled, etc.

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.

None yet

2 participants