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 unit tests #45

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Add unit tests #45

merged 2 commits into from
Jul 2, 2024

Conversation

lpsinger
Copy link
Member

No description provided.

@Courey
Copy link
Contributor

Courey commented Jul 1, 2024

Here is a gist that has a working version of the test in it.

There were a few changes:

  1. creating the process and cleaning up the process should be done in a before/after hook. This ensures we can do proper cleanup.
  2. I used spawn instead of execa. We're just streaming the output anyway so we don't really need a shell here. It doesn't block the event loop.
  3. I'm running it detached.
  4. I used beforeEach and afterEach so it will do the setup and cleanup on every test. You could do a before and after if you'd like to run multiple test on each setup.

@lpsinger
Copy link
Member Author

lpsinger commented Jul 1, 2024

4. I used beforeEach and afterEach so it will do the setup and cleanup on every test. You could do a before and after if you'd like to run multiple test on each setup.

We want to kill the process during the test because the functionality that is under test is that the process dies when it is supposed to.

@Courey
Copy link
Contributor

Courey commented Jul 1, 2024

  1. I used beforeEach and afterEach so it will do the setup and cleanup on every test. You could do a before and after if you'd like to run multiple test on each setup.

We want to kill the process during the test because the functionality that is under test is that the process dies when it is supposed to.

Then cut and paste that part into the test. It will have a small pause but still work as expected.

so you'd remove the after hook, and adapt the test so it looks like this:

it('starts and stops in sandbox mode', async () => {
  let result
  for (let i = 0; i < 30 && !result; i++) {
    try {
      const response = await fetch('http://localhost:3333/')
      if (!response.ok) throw new Error(`HTTP ${response.status}`)
      result = await response.json()
    } catch {
      await sleep(1000)
    }
  }
  assert.deepStrictEqual(result?.body.cluster_name, 'elasticsearch')
  if (childProcess.pid) assert.ok(process.kill(-childProcess.pid, signal))

  await new Promise<void>((resolve, reject) => {
    childProcess.on('close', resolve)
    childProcess.on('error', reject)
  })

  childProcess.stdin?.end()
  childProcess.stdout?.destroy()
  childProcess.stderr?.destroy()
})

lpsinger added a commit to nasa-gcn/.github that referenced this pull request Jul 2, 2024
Some projects use Jest for test coverage and some use the builtin
Node.js test runner. The two frameworks accept different CLI
options. Allow the calling workflow to customize the CLI options
for the test runner.

See nasa-gcn/architect-plugin-search#45.
lpsinger added a commit to nasa-gcn/.github that referenced this pull request Jul 2, 2024
Some projects use Jest for test coverage and some use the builtin
Node.js test runner. The two frameworks accept different CLI
options. Allow the calling workflow to customize the CLI options
for the test runner.

See nasa-gcn/architect-plugin-search#45.
@lpsinger lpsinger force-pushed the test branch 3 times, most recently from fe962ef to 6675f79 Compare July 2, 2024 13:38
@lpsinger
Copy link
Member Author

lpsinger commented Jul 2, 2024

@Courey, I did as you suggested: I switched from execa to child_process.spawn. That seemed to do the trick. I don't know what the deal is with execa. We've definitely had other problems with it.

I also did as you suggested with splitting out the cleanup and teardown code into before and after hooks; it turns out those can contain assertions. It made the code a lot simpler.

This is now passing on my Mac, but it's failing in the Windows CI. Did you and @dakota002 ever test your approach on Windows?

@Courey
Copy link
Contributor

Courey commented Jul 2, 2024

I believe we tested the previous version on Windows.
This failure?

# Error: Test hook "afterEach" at __tests__\\test.js:50:7 generated asynchronous activity after the test ended. This activity created the error "TypeError: Cannot read properties of undefined (reading 'on')" and would have caused the test to fail, but instead triggered an unhandledRejection event.

It looks like it's failing from the actual test suite itself.
It looks like the process is undefined for some reason here.

@lpsinger
Copy link
Member Author

lpsinger commented Jul 2, 2024

The process is undefined because it couldn't find the executable npx and never started. That's what this error message means:

      location: 'D:\\a\\architect-plugin-search\\architect-plugin-search\\__tests__\\test.js:65:7'
      failureType: 'hookFailed'
      error: 'spawn npx ENOENT'
      code: 'ENOENT'
      stack: |-
        ChildProcess._handle.onexit (node:internal/child_process:286:19)
        onErrorNT (node:internal/child_process:484:16)
        process.processTicksAndRejections (node:internal/process/task_queues:82:21)

@dakota002, help us!

@lpsinger lpsinger force-pushed the test branch 2 times, most recently from fc556a4 to bb78c0f Compare July 2, 2024 17:04
@lpsinger
Copy link
Member Author

lpsinger commented Jul 2, 2024

Actually, it's working now, even though I'm using execa. Mysterious!

@lpsinger
Copy link
Member Author

lpsinger commented Jul 2, 2024

@Courey, please review.

@lpsinger lpsinger merged commit 953469d into nasa-gcn:main Jul 2, 2024
10 checks passed
@lpsinger lpsinger deleted the test branch July 2, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants