steveloughran commented on PR #5982: URL: https://github.com/apache/hadoop/pull/5982#issuecomment-1701070887
i do think test age is an issue, and old code is suboptimal, but using test(expected) over our own intercept() or even assertThrows isn't something we'd do today. now, serious question for you: if a test is working, why does it need it maintenance? because there are other uses of engineering time, including * new features * bug fixes in production * writing new tests * reviewing patches by others * so, how would you justify the change, as "it's considered old fashioned" doesn't do it. if anyone was to go near old tests, i'd target things i really don't like 1. assertTrue/assertFalse without error messages or detail why the test failed. Fix: use assertJ assertions with .describedAs(), or at least add messages 2. assertEquals with the arguments reversed. fix, swap, or better: assertj. 3. code which uses try/catch/fail rather than intercept(). sadly too common moving #3 onto intercept() would be better than worrying about "expected"; while #1 is critical for debugging a test report "testXYZ failed on Line 222". It's why I automatically veto any PR where new tests don't add details. Still disappointed that people still submit PRs where they don't do that. very bad style. we need a stylechecker rule for it, at least for new code, -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
