gemmellr commented on PR #4615: URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1719061417
> create a server, upgrade and start the server is actually how you verify this issue. > The code needing tested is in neither the broker or client that test uses, the broker wasnt involved at all in the bug or the fix, so I disagree thats how it should actually be tested. The broker could still start even if a multitude of other highly similar/essentially-identical issues had occurred in the upgrade process. Using the broker+client to 'test' this by side-effect is both slow and gives a poor result. Spending 1 second (or whatever it actually is) every time the test suite is run in future while testing this type of issue, vs the comparitively much smaller amount of closely verifying the files / cli-handling, is essentially exactly how over time the test suite ends up in the state it is in now. > if you want to do the hard way (compare every single file) it will take longer to develop, and you still need to keep the system test. I wouldnt call it the hard way, but test development is often a time tradeoff yes. Development time now vs output quality and maintenance burden later. More time developing a good test, that runs fast, is reliable, is targeted, gives good failure messages, catches the entire general issue rather than just 1 variant that had a no-startup side effect...vs later having years of wasting time on every execution of slower poorer tests that verify by side effect, wont catch various near-identical related issues, dont give as nice a failure if they should fail, perhaps even fail sporadically....and which actually a lot of time people dont even bother to run precisely because the complete test suite is so slow and unreliable overall, which I know happens fairly regularly from the number of times I encounter it while reviewing PRs. > > As a matter of fact I'm almost done with a compare test.. it took me a couple hours to do it... (probably 3 or 4) which is way more than this test takes to execute (1 second each time). and I'm probably keeping the system test as well to validate future issues during the upgrade. I think this is a valid test. It might catch _this_ issue as a complete side effect, but it wont catch other almost identical issues, which the other test will have caught already. Its just a significantly poorer test overall relative to the alternative of checking the actual CLI behaviour to verify the CLI behaviour. We already have other tests starting the broker up with the same 'default config' the upgrade test should be verifying is what is still present after such a 'non-upgrade'. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
