#631: TeeInput test class is incomplete #698
Conversation
|
Job #698 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #698 +/- ##
============================================
+ Coverage 70.81% 74.31% +3.49%
- Complexity 1035 1101 +66
============================================
Files 221 222 +1
Lines 3348 3364 +16
Branches 190 190
============================================
+ Hits 2371 2500 +129
+ Misses 930 818 -112
+ Partials 47 46 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@proshin-roman This PR is too big and too raw. Splitting into PartA and PartB test classes doesn't feel OK.
Then, there are too many tests in one class, hard to find where an issue might be.
I also think you copy-pasted most of them and just changed some stuff -- since the PR is so big, it is very hard to review it and if there is a bug in one of them, boom, we have 1k tests with same bug.
See my comment, I think we should split these tests on categories.
Besides, don't forget about PDD, you clearly spent more than 30min on this task... the model will never ever repay you for overtime, keep that in mind!
Create a Test class for a category of tests (e.g. from Uri to ...), we will review that and you can create a puzzle for someone else to continue implementation of tests.
| "Hello, товарищ äÄ üÜ öÖ and ß"; | ||
|
|
||
| @Test | ||
| public void copiesFromUrlToPath() throws IOException { |
There was a problem hiding this comment.
@proshin-roman We have to create multiple test classes, since there are so many tests. An idea would be to create test classes based on the source of the TeeInput. E.g. TeeInputFromUriTestCase, TeeInputFromPathTestCase etc and each of these test files would contain tests for TeeInput from Uri to everything else, from Path to everything else etc.
|
|
||
| @Test | ||
| public void copiesFromUrlToOutput() throws IOException { | ||
| final Path input = Files.createTempFile("input", ".txt"); |
There was a problem hiding this comment.
@proshin-roman I think these temp files will automatically be deleted when the test is over or when the test fails/throws an exception, right?
There was a problem hiding this comment.
@amihaiemil No, these files are not deleted automatically. Method deleteOnExit has to be called to enable automatic deletion on JVM shutdown (more info here). Nevertheless, I found that no tests in Cactoos care about this problem, so all temp files stay forever in OS temp directory (until it's cleared by the OS itself). I will report a bug about this problem as it has to be fixed in many tests.
There was a problem hiding this comment.
@amihaiemil I've submitted #702
|
@amihaiemil I've added two puzzles about splitting test classes on small ones based on a source of the TeeInput. In the rest, I think this pull request fixes the original bug - low test coverage - and current test coverage is 100% for TeeInput. I'd suggest to approve this pull request and then splitting of the test classes will be done in two new tickets. |
|
@proshin-roman no, I am still not happy, sorry. I mean, puzzles and the concept of PDD is for continuing work on a certain topic, not fixing the quality of the previous puzzle. See the difference between the 2 concepts? If we merge this with those puzzles then we're basically saying: "hey, we wrote this pile of unit tests, please cleanup the mess and group the them" -- that's not how it works, each task has to be solved with a quality as high as possible. This PR has poor quality because it's simply too big, countless tests (I couldn't even count them) with no grouping etc. I haven't even reviewed them all, can't even guarantee that they are decoupled haha. So please, chose a group of tests, make a class for them and leave a single puzzle for continuing with other test classes. |
|
@proshin-roman or if you want, since you wrote them already, you could make more PRs with each group. This is up to you to decide. |
|
@proshin-roman and by the way, the convention in Yegor's projects is to name the branch as the ticket number it solves. Your branch is now names |
|
@proshin-roman ping; any status here? :) |
|
@amihaiemil thanks for your attention :) I'm gonna post a fixed version this evening |
|
@amihaiemil Pull request has been updated. I've fixed all comments. Please review it again. |
There was a problem hiding this comment.
@proshin-roman Too big again. Too much stuff. Please, see my comments. I could not review those 2 test classes yet (the first 2 test classes that I said I would accept), since I have no idea how that new matcher works.
Please, follow the methodology; be fast, efficient (even greedy if you must), don't spend too much on it. You spent on this what, 1 day and a half? Too much. Even if I accept this PR you only get paid 30min.
Small PRs and speed is the only way this model can work, since we do not have meetings, emails, phone calls etc. Besides, we have timezone differences.
If we make huge PRs traceability is gone, chances of bugs are enormous. Nobody can know what and where has been added etc.
And the scope of these projects (cactoos and jpeek) are to train you guys in the model. Don't be shy, don't be afraid that the project is moving too slowly or anything. It moves faster if the PRs are small and besides, we have to take all the time now, to make sure everybody understands how it all works.
| * @since 1.0 | ||
| */ | ||
| public final class TeeInputHasResult extends TypeSafeMatcher<TeeInput> { | ||
|
|
There was a problem hiding this comment.
@proshin-roman We did not discuss about this new Matcher, did we? It's nice that you did it, but there are no unit tests for it, how do we know it works? Countless new tests are relying on it to make assertions, but it (the matcher itself) is not tested at all.
Write some tests for it if you want to use it in these tests, otherwise please use simple Matchers.
Not to mention, this new matcher, together with unit tests for it, should be in a separate PR on a separate ticket, it's basically a new functionality of the library.
| * @checkstyle JavadocMethodCheck (100 lines) | ||
| * @checkstyle ClassDataAbstractionCouplingCheck (100 lines) | ||
| */ | ||
| public final class TeeInputFromByteArrayTest { |
There was a problem hiding this comment.
@proshin-roman Just this test class and the next one are enough for this PR. No new, untested matchers, no 13 changed files etc.
| @@ -0,0 +1,214 @@ | |||
| /** | |||
There was a problem hiding this comment.
@proshin-roman Again, 13 files. To quote Donald Trump: "Yuge PR". Make more PRs, please, Or you can add puzzles for future PRs, however you wish. For now, in the scope of this PR, I would only accept the 2 test files above.
|
@proshin-roman Also! Don't forget that it's all about tasks and money in this model. Nothing more: tasks and money. You spent a lot of time on this task, I imagine you are very frustarted (sorry about that). If you don't want to work on it anymore, you can just let it go, it's your choice. Of course, 0crat will "punish" you for it, but at the end of the day it's simple maths: what do you want? Only money? Can you afford to lose reputation? Then clearly leave it, since, you will earn on this task much less than you worked. On the other hand, if you also want to learn and push yourself forward, then finish it, but keep in mind this is not a school -- it's all really cold. Nobody cares about anything besides closing a task. Nobody will reward you if you work harder, do overtime etc. |
|
@amihaiemil Thanks for your effort on these explanations. I really appreciate it as you waste your time on this pull request as well. I'm trying to get this methodology working for myself, so I don't worry much about my extra time. Could you explain please which of these two options would be right then?
I guess the first option should be right, but the problem is that the ticket is still not fixed (test coverage is not 100%). |
|
@proshin-roman The first option is definetely better. Always close the ticket with puzzles, rather than adding impediments to it, if possible. Sometimes, there may truly be impediments to be added, but this is not the case. The original ticket (adding coverage of 100%) was simply too big for 30min. So, naturally, add some tests for it and puzzle the rest out. And don't forget about that new Matcher -- decide what you do with it? Want to write unit tests for it or use simple matchers? I would honestly use simple matchers since that is a new functionality, as I said, and then the ARCH will also take more time to review etc. Then, when the matcher is solved, I will have a last, short look over the tests 2 test classes :D They are short so shouldn't be a problem. |
|
@amihaiemil ok, thanks! it's getting better and clearer :) Gonna do the first option (without matcher of course - will create a new puzzle for that too). |
|
@amihaiemil I did that finally :) Review it again, please. |
|
@amihaiemil/z this job was assigned to you 5 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
|
@amihaiemil could you review this pull request again, please? It should be ok now. |
There was a problem hiding this comment.
@proshin-roman looks ok now. Just 2 comments about the puzzles formatting.
| @@ -45,6 +45,9 @@ | |||
| * @author Yegor Bugayenko (yegor256@gmail.com) | |||
| * @version $Id$ | |||
| * @since 0.1 | |||
| * @todo #631 This class needs more test cases. Currently, only a set of | |||
There was a problem hiding this comment.
@proshin-roman second and third line of this puzzle should start with a space. Also, add the estimation:
@todo #631:30min This class needs more test cases. Currently, only a set of
ctors is covered by tests: ctors which use Bytes and byte array as an
input. All other ctors should be covered too.
There was a problem hiding this comment.
@amihaiemil I've fixed both new comments. Btw: Qulice usually checks indentation for JavaDocs, but skips these two problems - that's a bit strange :)
| * @author Roman Proshin (roman@proshin.org) | ||
| * @version $Id$ | ||
| * @since 1.0 | ||
| * @todo #631 Create a new Matcher that will compare results of TeeInput as |
There was a problem hiding this comment.
@proshin-roman Same here, second line starts with a space and add estimation :30min
|
@rultor good to merge |
@amihaiemil Thanks for your request. @yegor256 Please confirm this. |
|
@proshin-roman and please, update the PR's description. Something like: "Pull request for the issue #631 Started adding tests for |
|
@yegor256 I also think we should have more test classes (1 per ctor input). So one for ByteArray ctors, one for Byte ctors etc. It is too much to have them all in one test class, it loses its purpose.. at least the purpose of documentation. People want to check out the unit tests and learn how to use the class; if they find a test class with 10k LoC, that's useless... |
|
@yegor256 besides, we should really find some balance, somehow. CR does a review, asks for changes, and then the ARCH comes and asks the exact oposite; this is not ok. But maybe a topic for discussion in another Issue :) |
|
@proshin-roman OK, let it be |
|
@rultor merge |
@proshin-roman @yegor256 Oops, I failed. You can see the full log here (spent 9min) |
|
@amihaiemil @yegor256 I've extended puzzle's description (didn't know about this constraint at all). |
|
@yegor256 can we try to merge again pls? |
|
@rultor merge |
|
@ypshenychka/z please review this job, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
@ypshenychka Can you review this one soon, there are only 2 days left |
|
@0crat quality good |
|
@ypshenychka Job |
|
@ypshenychka Job |
|
|
|
|
|
|
|
Order was finished, quality was "good": +20 points just awarded to @amihaiemil/z |
|
@ypshenychka The job #698 is now out of scope |
|
@0crat status |
Pull request for the issue #631.
Started adding tests for TeeInput's ctors. Added test cases for ctors which use Byte and ByteArray as inputs and left puzzles for continuing implementing tests.