tink icon indicating copy to clipboard operation
tink copied to clipboard

Rethink external runtime dependencies in unit tests

Open jacobweinstock opened this issue 3 years ago • 2 comments

The unit test code here creates docker containers for use in unit tests. This feels inappropriate for unit tests. Also the way it's written, all tests that call this function will get their own postgres container. As it currently stands, i have seen up to 4 containers running at a single time. This also feels inappropriate.

I would argue that unit tests should not require a container runtime to be installed. They should strive to only require Go.

I think that for functional or integration testing spinning up containers is acceptable. Maybe this code can be isolated to only be used for functional/integration testing.

Curious what others think?

jacobweinstock avatar Apr 14 '22 21:04 jacobweinstock

I remember writing those tests and I had similar concern. However, it was helpful testing the event system I wrote for Tink, and so it was acceptable at that point. But as I look at it now, I find it confusing and overcomplicated.

I believe we should rather define better abstractions and as you rightly said this code can be isolate for integration tests.

gauravgahlot avatar Apr 29 '22 11:04 gauravgahlot

I think that for functional or integration testing spinning up containers is acceptable. Maybe this code can be isolated to only be used for functional/integration testing.

Yes.

Unpopular opinion (possibly): Unit tests should also, ideally, run in 10s of ms (sub 100ms) and containers typically take more than .5s to launch.

chrisdoherty4 avatar May 03 '22 12:05 chrisdoherty4

Closed by https://github.com/tinkerbell/tink/pull/654

jacobweinstock avatar Dec 23 '22 01:12 jacobweinstock