This test HadoopFormatIOElasticIT is really used mostly as an integration test to test different Hadoop InputFormats and, afair, it came from the time when Beam didn’t have a dedicated Elasticserch IO connector. Though, if the test running time won’t be strongly increased after moving to test containers, I’d keep this test as it is now. Otherwise, we should make it as IT.
Btw, does this version bump introduce any user API incompatibilities? > On 13 Oct 2020, at 09:00, Piotr Szuberski <[email protected]> wrote: > > Yeah, I didn't mean it's a unit test as it'd already been quite heavyweight. > > It's much better than I thought - with the image pulled the test takes about > 5s compared to the previous 6-7s. I'll take this approach then, thank you for > your advice. > >> You could argue it's already an integration test. Running an integration >> test against an in-memory implementation doesn't make it a unit test (IMO). >> I think it's reasonable to run integration tests that execute against >> testcontainers in the PreCommit. The bar for PreCommit should be how >> expensive/time-consuming the test is compared to how much signal it gives >> us, not whether we classify it as a unit test. >> >> Have you compared the runtime when running the test with testcontainers? >>
