POC: Add setup and teardown hooks to CATs

[clnoll]

What

This demonstrates how we can add setup & teardown hooks to each test for which it's needed, to enable DB sources to setup the database before running read tests.

How

This approach shows how to configure acceptance-test-config.yml with setup & teardown scripts that will be run before/after each test for which they're needed. We use the ConnectorRunner to copy the scripts to the connector container and run them from there.

Tests that will run these scripts are marked with @pytest.mark.setup and @pytest.mark.teardown.

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jun 28, 2024 0:10am

[alafanechere]

Nice POC! I have two main questions:

• Will running commands inside the connector container provide enough system dependency / tools which might be required for the setup/teardown? If we move to rootless containers in a couple of week the users won't be root anymore and depedency installs via apt-get or yum won't be possible. I originally suggested to use a Dockerfile for maximal flexibility in terms of the environment that can be use to run these setup/teardown
• I don't fully get the requirement for markers here. I would suggest passing the setup_teardown fixture to the tests which need it. It will make it simpler to understand when / why the setup_teardown flow is called.

[alafanechere]

Suggested change

	@pytest.fixture(autouse=True)
	@pytest.fixture()

If I'm not mistaken autouse means the fixture code will always get executed, even if no test depend on it. I'm not sure this is what we want.

[clnoll]

Going to try scope="function" instead, which may be more like what we want.

[clnoll]

@alafanechere rather than passing in the setup_and_teardown fixture to each test where it might be needed, WDYT about having autouse=True here? The advantage is that it centralizes the logic for whether to run setup/teardown to the acceptance-test-config.yml. For any tests for which setup/teardown config isn't present, the fixture will be a noop, and we won't have to remember to mark or add the fixture to new tests.

Since DB sources is already going to have to add the setup config to the tests for which it's required in acceptance-test-config.yml, this feels like the cleanest approach.

[alafanechere]

🆗 . In any case we don't have to make it perfect right away and I don't have a better suggestion, let's ship it on figure out how well it's working for our use cases.

[alafanechere]

Does these marker mean that the setup and teardown logic will run for each test in the class or once per class instantiation?

[clnoll]

This marks each test in the class as having an option to do setup/teardown. When the setup_and_teardown fixture is being called for each test, it uses the presence of the marker to decide whether to run setup/teardown.

[clnoll]

@alafanechere to answer your questions:

Will running commands inside the connector container provide enough system dependency / tools which might be required for the setup/teardown?

This was definitely a concern if the script were to get very complicated at all. The latest commit addresses this - using a single setup/teardown Dockerfile that will be built by the CAT container.

I don't fully get the requirement for markers here. I would suggest passing the setup_teardown fixture to the tests which need it. It will make it simpler to understand when / why the setup_teardown flow is called.

The markers aren't strictly needed, we could definitely pass the fixture to the tests instead. I'll look into doing it that way before finalizing this PR. That said, the real way to tell whether the setup/teardown flow is happening will be by looking in the acceptance-test-config.yml. IMO the fixture has a similar drawback as the marker since it may or may not be a noop. But it is less code so is probably nicer.

[theyueli]

Thank you very much for this great work @clnoll !