Added new integration tests for event ingestion endpoint

[cmraible]

This PR adds a new integration test suite for the main event ingestion endpoint in the analytics service. It aims to create some new patterns that we can reuse in our other integration tests, so we can eventually replace the current tests in test/integration/app.test.ts.

There are two main blocks of tests:

• Request validation tests: these add a stubbed handler, so we can isolate the validation logic — we should get real validation errors from the app, or 202 if validation passes
• Batch mode tests: these spy on the pub/sub client library so we can send requests and spy on the pub/sub messages that are published, including which topic we publish to, and what data we include in the message

There are a few new patterns that should hopefully be nicer to work with than our current testing patterns:

• Using fastify.inject() to simulate sending HTTP requests to the app, without actually using HTTP
• A dedicated fixtures directory for simple JSON fixtures for request bodies, query params, and headers.
• Assertion utilities for grouping common assertions, i.e. expectValidationErrorWithMessage()
• A re-useable pubSubSpy so we can make assertions on when/what we publish to pub sub topics

There are a few tiny changes to the actual code here just for the sake of making testing easier.

[ibalosh]

reset is confusing me, but just a tiny bit :) maybe we could say here, createInstance ? it makes the intent more explicit or something like that?

[cmraible]

Yeah no disagreement. I also don't love this pattern in general — it's sort of trying to be dependency injection, but it's not really. I'll have more of a think about this.

[ibalosh]

Gotcha, ok, so I see you used Singleton pattern to create a single instance and reuse it over time. If I can read the intention in the code correctly, you use this single instance across in proxy. If that is the case, and it's always the same that makes sense.

But the reset seems out of place, and only used in tests. It could be taken out of this class in some ways, and used only in tests, or maybe something along the lines:

const googleProjectId = process.env.GOOGLE_CLOUD_PROJECT;
const productionPubSub = new PubSub({ projectId: googleProjectId });

class EventPublisher {
    private pubsub: PubSub;

    constructor(pubsub: PubSub) {}
    // Remove all singleton logic
}

const defaultPublisher = new EventPublisher(productionPubSub);

// Main API - used by production code
export const publishEvent = async (options: PublishEventOptions): Promise<string> => {
    return defaultPublisher.publishEvent(options);
};

// For tests - create with mock
export { EventPublisher }; 

In this case it's closer to the Dependency injection idea. Just thinking out loud as I was checking it out, feel free to check the comment out, you can resolve it too. Don't want to take too much time in the review from you.

[ibalosh]

Hey @cmraible

great job! I really love where this is going. Can you take one more look? We are close the the end for this one.

Most are suggestions, to keep things cleaner and tidier, one to focus on more is the pubsub-spy to clean up batch mode test.

Focus less on moving asserts out of tests, and keep them clean and easy to read, do not worry if they are longer, that is totally fine. We can always use multiple files if they grow out of hand, but since they are easy code to read, lenght is not as important as in codebase.

[ibalosh]

focus outside of implementation 👌 nice touch 👍

[ibalosh]

maybe nicer if we organize these with index.ts?

The more our code describes intentions (in this case, kind of the package, which would be fixtures) , the better.

Whatcha think?

import {
  defaultValidRequestQuery,
  defaultValidRequestHeaders,
  defaultValidRequestBody,
  headersWithoutSiteUuid
} from '../../../utils/fixtures';

[ibalosh]

up until here, I think it's fine, readable. you arrange and act. But here, couple of problems:

expectation is moved outside of the test

That can be fine, when intentions are clear, like the assertion you wrote for response. It's clearly visible you will assert responses, and in test itself, viewer already know mostly what you are asserting against.

In this case however, you moved the whole assertion logic, with conditionals to the spy, which is acting more as asserting framework, rather than just being a spy. Also, you are asserting waaay to many things there. I did that too before, so I know this feeling :) you just want to make it reusable, I understand.

But... What I would suggest is to go simple, move that logic back to the test. Also, go small, maybe you could try to test with same data, but add multiple tests and assert separate pieces: data, number of calls, timestamp. It's ok if they are separate tests, and there are repeated things, as long as test is readable. We could think about moving it back, fully, and start from there, see how it looks and start decoupling.

At the moment, I am not 100% sure what the test is doing, I do see the assertion name, but I am not sure what it does, and conditionals there confuse the reader. Tests should read simple.

I think that if you need multiple method names, and conditions in helper, it's a sign that test could be decoupled in multiple smaller intentional tests. You don't want to build in a separate DSL for assertion.

[ibalosh]

I have a feeling we don't need separate spy util at all

[ibalosh]

these look great! You might not even need describe('Request validation', function () { , up to you.

we can see in contexts after what is going on and in the file name.

If I would add one thing tiny one, I would add an empty line between expectResponse and above it, to distinguish AAA pattern (Arrange Act Assert).

[ibalosh]

will take another looks in next round, hope the comments above make sense, open comments are from the review, resolved all that is done done