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

Proposal: test.resource() #1366

Closed
jeffijoe opened this issue Apr 21, 2017 · 10 comments
Closed

Proposal: test.resource() #1366

jeffijoe opened this issue Apr 21, 2017 · 10 comments

Comments

@jeffijoe
Copy link

jeffijoe commented Apr 21, 2017

The problem

When integration-testing web apps (which AVA is great for since it runs tests in parallel), I sometimes run into issues with what I think is resource constraints, even on my high-end Macbook Pro. This results in the following sadness:

image

To fix this, I need to run AVA with --c 6 to lower the concurrency which is fine but if my MBP is using more resources for other things, I need to lower it even more.

The reason for this, is that I in each test suite run my HTTP server, set up a bunch of helpers for testing my API, connect to websockets, and then finally run my test. This is my pattern:

test('GET /todos returns all todos', withServer(async (t, api) => {
  const todos = await api.getTodos()
  t.true(todos.length > 0)
}))

Where withServer is just a factory function that sets up the server:

const axios = require('axios')
const createServer = require('../src/server')

function withServer (testFn) {
  return async function (t) {
    const server = (await createServer()).listen()
    const host = `http://127.0.0.1:${server.address().port}`
    const client = axios.create({ baseURL: host })
    const api = {
      getTodos: () => client.get('/todos').then(r => r.data)
    }
    
    await testFn(t, api)
  }
}

This makes it super easy for me to write integration tests for my API. Problem is, each test need their own connection to the server (in case of websockets), and even when I memoize the server (which only works per suite due to each suite being in a separate process), I still run into the above issue.

I think this is because I am using rethinkdbdash which manages a large connection pool. For a single server instance or 4 that might be fine, but for 10+, that might be why it starts having issues.

The proposed solution

What I would really like to do, is set up just a single instance for all my tests to work with.

One way to do it is to run npm start & npm test, but if npm start takes too long to actually start the server, then that's gonna be a problem.

Instead, what if we had a way to tell AVA to run a JS function before starting the test processes? For example, if I was able to do this:

// integration-test.resource.js
import test from 'ava'
import createServer from '../src/createServer'

// This runs before any other test processes have been spun up
test.resource('Set up API server', async (t) => {
  const server = (await createServer()).listen()
  const host = `http://127.0.0.1:${server.address().port}`
  
  // Only JSON-serializable values can be provided to test processes
  t.resource('serverHost', host)
  
  return async function dispose () {
    // When all tests are done, dispose of the resource
    server.close()
  }
})

And in my test

test('GET /todos', async (t) => {
  const client = axios.create({ baseURL: t.resource('serverHost') })
  const { data } = await client.get('/todos')
  t.true(data.length > 0)
})

The result is a single HTTP server that is ready when tests run, and a way to tear it down once all tests have run (pass or fail, does not matter). This is way more CI friendly than the npm start & npm test approach.

Proposed Configuration

To load these resources via CLI:

ava 'test/*.test.js' --resources 'test/resources/*.resource.js'

Via package.json

{
  "ava": {
    "resources": [
      "test/resources/*.resource.js"
    ]
  }
}
@novemberborn
Copy link
Member

Interesting. If I'm understanding this correctly you're proposing a "helper worker" that AVA manages for you, and that can expose some config that test workers can read.

@jeffijoe
Copy link
Author

jeffijoe commented Apr 21, 2017

Yes, exactly. Helper worker is an even better name for it.

It will make AVA more scalable in the sense that I don't have to consolidate a bunch of assertions into a single test case just to avoid the extra overhead.

Setting up API servers for each test also takes a while; most of the time I want to wait for certain connections to be made before I start listening, adding startup time to each test case. If I could do this once per run, that would cut down test run time dramatically.

@ronen
Copy link
Contributor

ronen commented Nov 20, 2019

I still think this would be a great thing to have. But I'd like to propose a minor tweak: Optional concurrency limits for access to the resource:

  • The access to the resource in a test would be async and would require await. That async access would block if the concurrency limit was reached, waiting until the resource was available again. E.g. changing the OP's example to:

    test('GET /todos', async (t) => {
      const client = axios.create({ baseURL: await t.resource('serverHost') })
      const { data } = await client.get('/todos')
      t.true(data.length > 0)
    })
  • The code that sets up the resource would be able to optionally specify a concurrency limit. E.g. modifying the OP's example, this could be done as:

    // This runs before any other test processes have been spun up
    test.resource('Set up API server', async (t) => {
      const server = (await createServer()).listen()
      const host = `http://127.0.0.1:${server.address().port}`
    
      // Only JSON-serializable values can be provided to test processes
      t.provide('serverHost', host, { concurrency: 8 })
    
      return async function dispose () {
        // When all tests are done, dispose of the resource
        server.close()
      }
    })

    At the risk of bikeshedding, I've also changed t.resource here to t.provide to avoid overloading with t.resource in the test.

    More bikeshedding could include: whether specifying the concurrency belongs in the config file/CLI rather than in the setup code; whether there should be an easy way to set it to relative to the number of CPU cores; and whether a concurrency limit should be enabled by default for all such resources.

(See also discussion at #2048 (comment))

@novemberborn
Copy link
Member

Sounds great. Would you have time to work on this stuff @ronen?

@ronen
Copy link
Contributor

ronen commented Nov 23, 2019

Would you have time to work on this stuff @ronen?

Would love to, but sadly very unlikely to find the time. At least not for the forseeable future. 😞

@lewisdiamond
Copy link

I think this is a great start. I've been looking for well integrated solutions to a similar resources issue we have in our test harness. The interesting thing about this solution is that it's simple to implement (given the JSON serialization and static nature of the data passed to workers). However, it also limits the capabilities.

Currently, each of our test create a new cassandra keyspace and initializes our schema (like a database / table space in SQL + creating the schema). Unfortunately, this operation is somewhat (surprisingly) costly. Using pre-test resource initialization would allow us to create a keyspace pool prior to starting the tests (most likely feeding off of the -c argument to know how many keyspaces to create). The problem is that we'd need inter-process communication for each test or each worker to request and lock a keyspace.

I understand that this would add a lot of complexity to AVA. Maybe someone can suggest a better (simpler) approach. Our short-term solution is a per-file keyspace pool.

@ronen
Copy link
Contributor

ronen commented Feb 28, 2020

@lewisdiamond wrote:

Using pre-test resource initialization would allow us to create a keyspace pool prior to starting the tests (most likely feeding off of the -c argument to know how many keyspaces to create). The problem is that we'd need inter-process communication for each test or each worker to request and lock a keyspace.

Very good point! The API described in the earlier comment allows the resource creator to provide the same value to each client. But a resource creator should be able to provide a different value to each client from a pool of precomputed values or, more generally, generating values dynamcally. I think it would be possible to handle that keeping the above API but with options to t.provide() to support different scenarios. Perhaps something like:

// Provide a shared JSON-serializable value `host` to up to 8 parallel clients
t.provide('serverHost', { 
 value: host,
 concurrency: 8,
}) 

// Provide a JSON-serializable value from a pool, to up to 8 parallel clients
t.provide('keyspace', {
 pool: [{ host, key: key1 }, { host, key: key2 }, ..., { host, key: key8 }],
})

// Specify a `create` function that creates a JSON-serializable value to provide
// to a client upon request, with up to 8 parallel clients.  When a client finishes,
// the optional `release` function will be called with that same value, allowing
// the resource provider to tidy up as necessary.
t.provide('workspace', { 
   create: workspaceCreator,
   release: workspaceCloser,
   concurrency: 8,
}) 

Of course with a general create/release mechanism, the others are just convenient shorthands:

// Provide a shared value
t.provide('serverHost', { 
  create: () => host,
  concurrency: 8,
})

// Provide values from a pool
const pool = [{ host, key: key1 }, { host, key: key2 }, ..., { host, key: key8 }]
t.provide(`keyspace`, {
  create: () => pool.pop(),
  release: val => pool.push(val),
  concurrency: pool.length,
})

Also, to be able to provide a resource per test file rather than per test as per this issue, the API should allow getting a resource in a .before() hook:

test.before(t => {
  t.context.keyspace = t.resource('keyspace')
})

where the resource will automatically be released when all tests in the file finish.

[And sorry, I still don't have time to actually implement this. Just kibbitzing... 😞 ]

@ronen
Copy link
Contributor

ronen commented Feb 28, 2020

Minor additional thought on the proposed API: In the above examples, AVA doesn't know about the serverHost keyword until after the server has successfully spun up and t.provide() is called, making it hard for AVA to administer the resources.

Instead, I think the resource keyword should be supplied to test.resource() rather than to t.provide():

test.resource('myServer', async (t) => {
  const server = (await createServer()).listen()
  const host = `http://127.0.0.1:${server.address().port}`
  t.provide({ value: host, concurrency: 8 })
  return async function dispose () {
    server.close()
  }
})

This way:

  • As soon as AVA executes all the test.resource() methods, it will immediately know all the resource keywords. It wouldn't need to wait for the async resource definition methods to resolve, it could leave them running asynchronously and start executing the tests

  • When a test calls await t.resource(keyword), AVA will know immediately if no such resource were defined, and could immediately fail. Otherwise it would block until that resource calls its t.provide() method.

  • The configuration files/CLI could allow configuration for specific resource to be keyed by its keyword. E.g. something like:

    {
       "resources": {
           "myServer": { 
                   "concurrency": 4,
                   "params": { "...": "arbitrary data" }
            },
       }
    }

    And then the execution context (t param) to the resource definition could include that configuration info, allowing const server = (await createServer(t.params)).listen() or whatever.

@vintprox
Copy link

vintprox commented May 17, 2020

This is good idea! Let's say, it's very useful for running Nuxt.js server with all required modules (specified in nuxt.config.js) for test purposes and disposing of it.

I've made a little memo on how I would set up Nuxt.js server with Ava's test.resource: nuxt/typescript#293 (comment)

@novemberborn
Copy link
Member

AVA now has experimental shared worker support. Code runs in a worker thread, in the main process, and can communicate with test workers.

The proposals made in this issue could be implemented using this infrastructure. Please join us in #2605.

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

No branches or pull requests

5 participants