emilssolmanis commented on pull request #14272: URL: https://github.com/apache/beam/pull/14272#issuecomment-811917880
@TheNeuralBit yeah I saw that, and didn't understand why they'd done it. I'd be keen to hear from the author themselves @rmannibucau if you have context, but my guess would be both JUnit5 and Beam have evolved in the 2 years since. Purely just implementing the before / after `TestExecutionCAllback` interfaces from JUnit5 seems to work, the added JUnit5 test suite verifies the same things the JUnit4 ones does, this includes the various calls to `PAssert`. Anecdotally I also did some local testing on a company project where we're looking to utilise this by copy-pasting all the relevant bits and just implementing the 2 JUnit5 extensions, and all our tests seem to pass by just swapping out the annotations and runner. tl dr there's a test suite exercising `PAssert` calls and local testing would seem to confirm that the code is fine. I don't mind an extension, since that solves the whole "clients would be required to depend on both versions of JUnit" debacle, but not sure that's justified for 2 classes (the TestPipeline and the sys prop restorer util). If that's the preferred route I'd love some thoughts on whether we want to just copy-paste the existing TestPipeline and allow for potential divergence of the JUnit4 / 5 implementations, or whether the current inheritance coupling seems appropriate. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
