review: test: add tests for JavaOutputProcessor class #4043
Conversation
There was a problem hiding this comment.
Tests look good to me, just had a little nitpick on the file exists assertion.
On a side note, with JUnit5, test methods do not need to be public anymore, it's sufficient that they are package private. I don't think that matters too much in the grand scheme of things, but I'd personally make the test methods package private.
There was a problem hiding this comment.
Looks good, very well written tests @Rohitesh-Kumar-Jain! I'd encourage you to look at the first tests you wrote, and then look at these. The improvement is tangible.
I had one nitpick on your edit. It's such a small thing that I'll leave it up to you if you want to change it. Will merge tomorrow morning regardless.
|
|
||
| // act | ||
| javaOutputProcessor.process(module); | ||
| File expectedFile = tempDir.toPath().resolve("emptyModule/module-info.java").toFile(); |
There was a problem hiding this comment.
This is really nitpicking, but in the AAA pattern, computing an expected value is not part of the act phase. It either belongs in arrange or assert, depending on who you ask. For a value as simple as this one, I'd definitely put it in the assert phase to keep the variable use close to its definition.
I am glad I am improving 🎉
Yes, actually I look often at my previous tests and suggestions requested for them, Idea is avoiding the past mistakes, and learn from new mistakes. |
There was a problem hiding this comment.
Looks good, well done!
FYI, I'm going on vacation tomorrow and will be back on August 9th. I may be around Spoon a bit but certainly not every day. So, probably expect slower feedback during this time.
@nharrand Ping that I'm going away.
#1818
Hi, I have added some new tests for methods of the
JavaOutputProcessorclass :These tests are intended to kill these 2 mutations:
Link to #3967