[SPIKE] Basic Environments Testing

[casesandberg]

This isn't an exhaustive list, but mainly a showcase of various examples that can be utilized for testing both runtimes and frameworks. Let me know what you think about the direction. I believe this pattern can be applied to test most of the environments we've discussed.

Approach:

I adopted a "real-world scenario" approach. Instead of mocking or simulating environments, I set up actual projects (fixtures) for each environment. The tests then interact with these projects, running them, linking the built package, and checking for expected behaviors.

TODO:

• Mock the LLM network call to prevent tests timing out without obscuring the testing of library internals.
• Fix the package linking inside next-app test.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
llama-index-ts-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 27, 2023 4:02am

[sweep-ai]

Apply Sweep Rules to your PR?

• Apply: Leftover TODOs in the code should be handled.
• Apply: All new business logic should have corresponding unit tests in the tests/ directory.
• Apply: Any clearly inefficient or repeated code should be optimized or refactored.

[casesandberg]

Looking at some prior art from the environment testing in langchainjs, it seems they primarily focus on testing static imports, dynamic imports, and certain internal methods related to vectors and their CSV loader. We could adopt a similar strategy, which would allow us to avoid the complexities of mocking or making actual queries within the fixtures.

[yisding]

Thanks for doing this @casesandberg. I agree yeah the primary test should be about imports and exports working. The one issue with testing the output of the LLMs directly is that they're non-deterministic.

[yisding]

I'm going to merge this. Thanks!

[yisding]

So the first time I run pnpm run test I get this:

llamaindex:test: FAIL src/tests/frameworks/next-app.test.ts (12.633 s)
llamaindex:test:   ● Next App Router › Node Runtime
llamaindex:test:
llamaindex:test:     thrown: "Exceeded timeout of 10000 ms for a hook.
llamaindex:test:     Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."
llamaindex:test:
llamaindex:test:        8 |   let server: ExecaChildProcess;
llamaindex:test:        9 |
llamaindex:test:     > 10 |   beforeAll(async () => {
llamaindex:test:          |   ^
llamaindex:test:       11 |     port = await getPort();
llamaindex:test:       12 |     fixture = await createFixture("./src/tests/fixtures/next-app");
llamaindex:test:       13 |
llamaindex:test:
llamaindex:test:       at src/tests/frameworks/next-app.test.ts:10:3
llamaindex:test:       at Object.<anonymous> (src/tests/frameworks/next-app.test.ts:5:1)
llamaindex:test:
llamaindex:test:
llamaindex:test:   ● Test suite failed to run
llamaindex:test:
llamaindex:test:     TypeError: Cannot read properties of undefined (reading 'kill')
llamaindex:test:
llamaindex:test:       32 |
llamaindex:test:       33 |   afterAll(async () => {
llamaindex:test:     > 34 |     server.kill();

and then on subsequent times I get this:

llamaindex:test: PASS src/tests/frameworks/next-app.test.ts (8.275 s)
llamaindex:test: A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.

[casesandberg]

@yisding I wonder if its because the dependencies aren't installed for the fixture app. Let me look into it. May just need to increase the timeout for that setup.

[casesandberg]

@yisding Its likely that the cold start of the next server was straddling the timeout, so I increased it. The warning about a worker process not exiting gracefully was from that first attempt you made where it timed out and didn't clean itself up. Its safe to ignore that and will go away.

[yisding]

OK so it's still hanging CI and giving me that error. Command: npx jest --runTestsByPath src/tests/frameworks/next-app.test.ts

I tried await fixture.rm() but that threw a bunch of other errors.

I think maybe Jest might not be the right runner for these tests and maybe we should just go to using a shell script or something.

[casesandberg]

Thanks for working with me on this. Turns out its tricky to spawn nextjs from within jest from within turbo. You were on the right path with cleaning up the fixture. Turns out the Next app spawns a bunch of child processes and server.kill() isnt effective at killing all child processes, so I've utilized the terminate package that loops through all child processes to ensure they are killed.