test: add backend integration tests for server and analytics plugins#79
test: add backend integration tests for server and analytics plugins#79calvarjorge wants to merge 5 commits intomainfrom
Conversation
…ugins Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
There was a problem hiding this comment.
What was the reason of refactoring the server.integration.test.ts?
Personally I like to keep tests close to the actual code - maybe we should revisit the previous structure?
There was a problem hiding this comment.
I usually followed the pattern of having unit tests next to the actual code and integration tests in their own package, since they're usually run together and have common componentes. For instance, a typical flow in the CI would be to run all unit tests and, if those pass (or in parallel to that job), have a job run the integration test package.
Happy to have them in their respective dir, since we only have two plugins, and if needed we can re-visit later.
There was a problem hiding this comment.
Is it possible to test it similarly to the deleted server.integration.test.ts? That is, we could add the analytics.integration.test.ts close to the analytics plugin, and maybe we don't need the actual test server? WDYT?
There was a problem hiding this comment.
Sure, I undid the changes of server.integration.test.ts. My reasoning was that since the server is a common component of all integration tests, I would be nice to abstract it to a helper func.
Resolve conflict by keeping main's simpler server.integration.test.ts
a0fb1ad to
bd9fb65
Compare
Address PR review feedback: - Delete centralized integration test infrastructure (src/tests/integration/) - Keep original simple server.integration.test.ts (from main) - Add analytics.integration.test.ts colocated with analytics plugin - Use existing mockServiceContext pattern for simpler test setup
bd9fb65 to
31dd57b
Compare
| server = app.server.getServer(); | ||
| baseUrl = `http://127.0.0.1:${TEST_PORT}`; | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
Not needed - was meant to give the server a bit of time to start up, but I see tests run fine without it. Removed it.
createTestServer()utility for integration testssrc/tests/integration/