fix: use testcontainer for write tests

[jdpgrailsdev]

What

• Use MSSQL testcontainer for write integration tests

How

• Configure write integration tests to use MSSQL testcontainer
• Ensure that configuration is updated before each integration test
• Support replacement of values in already deserialized configuration

Review guide

1. BasicFunctionalityIntegrationTest.kt
2. MSSQLConfiguration.kt
3. MSSQLContainerHelper.kt
4. MSSQLWriterTest.kt

Can this PR be safely reverted and rolled back?

• YES 💚
• NO ❌

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 17, 2025 2:50pm

[edgao]

I'm still confused about the parallel tests fighting each other thing - in principle, every test is operating on <random namespace>.<hardcoded stream name(s)>? So they shouldn't ever be touching the same MSSQL table. Kind of worried that randomizing the stream name is just papering over some underlying problem

otherwise lgtm 🚛

[edgao]

is this still necessary?

[jdpgrailsdev]

I'm still confused about the parallel tests fighting each other thing - in principle, every test is operating on <random namespace>.<hardcoded stream name(s)>? So they shouldn't ever be touching the same MSSQL table. Kind of worried that randomizing the stream name is just papering over some underlying problem

otherwise lgtm 🚛

@edgao From what I discovered, I think that the issue is that the random schema is per test suite, not per test. Therefore, each test uses the same schema name AND the same stream name before this change. If that is not the intent, we should fix the base integration test classes to use a random schema name per test and not per suite.

[edgao]

ugh. There's probably some magic junit configuration that I forgot about somewhere... I'm pretty sure (85%) that s3/iceberg are creating a new instance of the test class per test method execution :/

[edgao]

actually derp, I missed this on my readthrough - why do we need this? I'm pretty sure this is the problem behind #51594 (comment)

(i.e. if you remove this, it should work as expected, b/c the default is PER_METHOD)

[jdpgrailsdev]

ugh. There's probably some magic junit configuration that I forgot about somewhere... I'm pretty sure (85%) that s3/iceberg are creating a new instance of the test class per test method execution :/

@edgao I can look at the iceberg tests tomorrow to see if I missed something. It may also be that we had to use per class test instances to avoid starting and stopping the test container after each test. If that is what is causing this, I'd rather use the random stream name that incur the cost of spin up/spin down of the container on each test.

[edgao]

the pattern we've used in other tests is to actually share the test container across test classes, via some singleton object. roughly

object SharedTestContainer {
  private val isStarted: AtomicBoolean
  fun start() { ... }
}

class TestClass1 {
  @BeforeAll fun setup() { SharedTestContainer.start() }
}

class TestClass2 {
  @BeforeAll fun setup() { SharedTestContainer.start() }
}

e.g. https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/destination-s3-data-lake/src/test-integration/kotlin/io/airbyte/integrations/destination/s3_data_lake/NessieTestContainers.kt. IIRC the old postgres/mysql destinations also do something along these lines

(we don't need to explicitly call testContainer.shutdown(), because testcontainers spawn a ryuk container, which kills the testcontainer automatically when it detects the JVM process exit)

[jdpgrailsdev]

the pattern we've used in other tests is to actually share the test container across test classes, via some singleton object. roughly

object SharedTestContainer {
  private val isStarted: AtomicBoolean
  fun start() { ... }
}

class TestClass1 {
  @BeforeAll fun setup() { SharedTestContainer.start() }
}

class TestClass2 {
  @BeforeAll fun setup() { SharedTestContainer.start() }
}

e.g. https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/destination-s3-data-lake/src/test-integration/kotlin/io/airbyte/integrations/destination/s3_data_lake/NessieTestContainers.kt. IIRC the old postgres/mysql destinations also do something along these lines

(we don't need to explicitly call testContainer.shutdown(), because testcontainers spawn a ryuk container, which kills the testcontainer automatically when it detects the JVM process exit)

@edgao That doesn't work in Kotlin. You cannot use BeforeAll/AfterAll on non-static methods. The only way for that to work is to mark the test instances as per class so that it can use the non-static methods. From looking at the Iceberg tests, it looks like the tests were also marked as JvmStatic to work around this, which is a bit of a hack. I can make that change to this PR and back out the random stream name change to see if that works.

[jdpgrailsdev]

@edgao I am able to make the tests pass without the random stream name by using the @JvmStatic + companion object trick. That being said, this is definitely a bit of an anti-pattern to put the before all/after all stuff in a companion object just to work around this issue. I don't see why we need to use the same stream name for all tests. Randomly generating one per test would allow us to make this more Kotlin-y and avoid this hack.

[edgao]

medium strength opinion: I don't see this as a hack. JvmStatic is annoying, yes, but that's more a function of us using junit in kotlin than anything? If we were on kotest, it would be a perfectly normal beforeSpec block. (I think? haven't actually used kotest myself)

also, I think our tests are already sufficiently complicated... Randomizing the namespace was annoying, but probably unavoidable - I'd like to avoid adding even more dynamicly-generated stuff when possible

and more broadly:

• we all agree starting+stopping testcontainers is expensive
• we all agree starting a testcontainer per method is probably a nonstarter
• starting a testcontainer once per gradle run instead of once per test class feels like the logical progression?
  • in particular - it removes the incentive to put all our tests in a single class for the sake of reducing infra overhead

[jdpgrailsdev]

medium strength opinion: I don't see this as a hack. JvmStatic is annoying, yes, but that's more a function of us using junit in kotlin than anything? If we were on kotest, it would be a perfectly normal beforeSpec block. (I think? haven't actually used kotest myself)

also, I think our tests are already sufficiently complicated... Randomizing the namespace was annoying, but probably unavoidable - I'd like to avoid adding even more dynamicly-generated stuff when possible

and more broadly:

• we all agree starting+stopping testcontainers is expensive

• we all agree starting a testcontainer per method is probably a nonstarter

• starting a testcontainer once per gradle run instead of once per test class feels like the logical progression?

  • in particular - it removes the incentive to put all our tests in a single class for the sake of reducing infra overhead

@edgao I guess I was referring to the use of a companion object to work around this as a hack, which it is -- its to make this work with Java-based JUnit. That being said, I don't feel strongly about it. I'd rather have us agree upon what level should be dynamically generated. It is currently the namespace and it sounds like we don't believe that it needs to be done at the name level of the stream. I'm not sure that I am sold on that yet, but given that it works now that I have introduced the companion object and static designation, we can pause this discussion for now. Thanks for the review/feedback.