| *** mlncn has joined #npoacct | 00:56 | |
| *** bkuhn has joined #npoacct | 04:28 | |
| *** bkuhn has quit IRC | 06:18 | |
| *** mlinksva has quit IRC | 10:50 | |
| *** tgnit has joined #npoacct | 16:17 | |
| *** tgnitidle has quit IRC | 16:20 | |
| *** bkuhn has joined #npoacct | 17:38 | |
| bkuhn | tgnit: greetings. | 17:40 |
|---|---|---|
| tgnit | bkuhn: greetings! | 17:41 |
| bkuhn | tgnit: have you thought more about how you want to proceed? | 17:43 |
| tgnit | i am writing unit test for balance. cc :) | 17:45 |
| bkuhn | Ok, that makes sense. | 17:46 |
| bkuhn | I'm sorry that the feature was so complicated. | 17:46 |
| bkuhn | tgnit: One thing I would like you to do before summers' end as well then... | 17:46 |
| tgnit | bkuhn : i am done with sorting balance object by commodity will wait for decision taken on mailing list | 17:46 |
| bkuhn | .... and I know writing "papers" is tough ... | 17:46 |
| bkuhn | ... is write a one-page report on what you tried. | 17:46 |
| tgnit | sure! | 17:46 |
| bkuhn | thanks. | 17:47 |
| bkuhn | tgnit: don't worry; this feature turned out to be MUCH more complicated than we expected. | 17:47 |
| bkuhn | I talked some with johnw about it. | 17:47 |
| tgnit | what he said? | 17:47 |
| bkuhn | Well, mainly that it's not clear what this feature should do. | 17:47 |
| bkuhn | That we're all confused about the implications. | 17:47 |
| bkuhn | Which I think both tbm and I are. | 17:48 |
| bkuhn | We keep finding new issues that he and I didn't anticipate. | 17:48 |
| bkuhn | johnw is reluctant to add any feature upstream that we don't understand all the use-case and implications of. | 17:48 |
| bkuhn | This happens sometimes in Free Software. | 17:48 |
| bkuhn | You did a good job, sometimes, your patches don't get accepted upstream. | 17:48 |
| bkuhn | But, you're right to do it: | 17:48 |
| bkuhn | Prepare the "sorting version" as a final proposal. | 17:49 |
| bkuhn | Go ahead and *do* submit a merge request, "for the record". | 17:49 |
| bkuhn | But understand that johnw will probably have to reject it. | 17:49 |
| bkuhn | That's not failure, many many changes get rejected in Free Software. :) | 17:49 |
| tgnit | bkuhn: i understand! | 17:49 |
| bkuhn | I mean, to be honest, it *is* failure, but it's not a failure of your GSoC time: you've learned that sometimes, you write your best patch and upstream won't take it. ;) | 17:49 |
| bkuhn | There has been a lot written that we learn well from failure. | 17:50 |
| bkuhn | I think you did your best, and I am happy with your work. | 17:50 |
| bkuhn | I can imagine I'd have done the same approach myself and get it rejected. | 17:50 |
| bkuhn | If you had more time for GSoC, we'd have you try again. | 17:50 |
| bkuhn | So, tgnit, thus the plan is: | 17:50 |
| tgnit | bkuhn: i think johnw is right by doing it, you can't merge a thing you can't explain | 17:50 |
| bkuhn | :) | 17:50 |
| bkuhn | so the plan is: | 17:50 |
| bkuhn | 0. You'll finish the patch, upstream it, and johnw will likely reject. | 17:50 |
| bkuhn | 1. You'll write a one-page paper about your experience preparing the patch, what you learned, etc. | 17:51 |
| bkuhn | 2. You'll write at least some tests in the final weeks that are good enough to get merged upsteram. | 17:51 |
| bkuhn | *upstream | 17:51 |
| bkuhn | If you do 0-2 successfully, I will pass you for GSoC. | 17:51 |
| tgnit | bkuhn: got you | 17:52 |
| bkuhn | For (2), I suggest upstreaming the tests one-by-one, or in small groups of 1-2 tests. | 17:52 |
| tgnit | thanks for making it clear | 17:52 |
| bkuhn | Sure! I think you've done a reasonable job. Neither tbm nor I understood how complicated this feature would be. I believe you did your best. | 17:52 |
| bkuhn | Ok, let me know if you need any help or questions on 0-2 above. | 17:53 |
| tgnit | bkuhn: thanks ! | 17:53 |
| bkuhn | Let's check in again on, say, Thursday, to see where you are on that? | 17:53 |
| tgnit | sure! | 17:53 |
| tgnit | can you check code_coverage branch for once? | 17:54 |
| tgnit | on https://github.com/tripun/ledger | 17:55 |
| bkuhn | sure, what specifically do you want me to look at? | 17:55 |
| * bkuhn does a pull | 17:55 | |
| tgnit | just the latest commits. | 17:55 |
| tgnit | unit test on balance. cc | 17:55 |
| tgnit | i have tried they are working fine | 17:56 |
| bkuhn | Oh, I see johnw accepted your change for acprep! Good work! :) | 17:57 |
| bkuhn | I see you're adding unit tests to the C++ unit test suit. | 17:58 |
| bkuhn | *suite | 17:58 |
| bkuhn | that's fine. | 17:58 |
| bkuhn | I was thinking more about adding tests to the regression tests too. | 17:58 |
| bkuhn | I guess there is no code-coverage run of the regression tests? | 17:59 |
| bkuhn | I.e., is there a report that can be generated that show how much the regression tests cover the code? | 17:59 |
| bkuhn | Or does the test report actually show coverage of the entire unit test + regression tests? | 17:59 |
| bkuhn | I actually don't know (I should, I know :) | 17:59 |
| bkuhn | I've added lots of tests to Ledger, but I never ran the coverage report myself. :) | 17:59 |
| tgnit | there is code coverage but i have not generated new report | 18:00 |
| tgnit | bkuhn: i started with balance. cc as the unit that was empty | 18:00 |
| bkuhn | Yeah, that's smart. | 18:03 |
| bkuhn | So, I think you should do a mix, though, of regression and unit tests. | 18:04 |
| bkuhn | to boost coverage. | 18:04 |
| tgnit | ok | 18:04 |
| tgnit | i need to check as to what tests are missing in regression though | 18:05 |
| bkuhn | Yeah, I am still unclear; is there a report that does a gcov report for the regression tests, and/or "the regression and unit tests together"? | 18:05 |
| tgnit | its a total report | 18:10 |
| bkuhn | ah, ok. | 18:10 |
| bkuhn | that's good. | 18:10 |
| bkuhn | So, I'd encourage you to add a mix of unit tests and regression tests. | 18:10 |
| bkuhn | I'd suggest as above to offer merges to johnw every 2-3 days. | 18:10 |
| tgnit | need to check how to get coverage only for specific tests | 18:10 |
| tgnit | ok | 18:11 |
| bkuhn | Yeah, I'm not *terribly* worried if we get coverage reports for specific tests, if the coverage report is for the whole suite already. | 18:11 |
| bkuhn | Just write tests of all types. | 18:11 |
| tgnit | sure! | 18:12 |
| bkuhn | tgnit: for the moment, maybe write a few more unit tests /test/unit/t_balance.cc and submit that file as a merge request? | 18:13 |
| bkuhn | Then, while you're waiting for johnw to review that, work on some new regression tests. | 18:14 |
| bkuhn | That way if johnw has feedback on your unit tests that require a change in how you're doing them, you won't have too much extra work to fix. | 18:14 |
| tgnit | right | 18:14 |
| bkuhn | then, you hopefully will have regression tests to merge, and you can keep switching back and forthn. | 18:14 |
| tgnit | that is right way to do it. | 18:15 |
| tgnit | bkuhn: the sorting and rounding stuff should fix the totalling problem, i do not anticipate issues with monetary use then, although it may not be merged but it may replace your workarounds! what do you think? | 18:25 |
| tgnit | there may still be issues but it should do the minimum function expected | 18:25 |
| bkuhn | tgnit: Yes, I think it will not be merged. But, as I said, it's worth submitting the pull request, and let johnw formally reject it. That will create a formal record in Github of what we tried and why johnw didn't accept it. This will be useful for people who want to use the patch separately. | 18:26 |
| bkuhn | tbm and I may use it ourselves, even. :) | 18:27 |
| bkuhn | I have not run Conservancy's books against your patch -- I was waiting for it to be merged. | 18:27 |
| bkuhn | But I'll try once it's rejected and see what happens. :) | 18:27 |
| tgnit | bkuhn: nice to know that :) | 18:28 |
| bkuhn | tgnit: at the moment, though, I have written work arounds in our books for all rounding errors by changing the numbers slightly | 18:28 |
| bkuhn | i'll look for some of those and see if changing them back works with your patch, too | 18:29 |
| bkuhn | but, let's see what johnw says officially first. | 18:29 |
| tgnit | ok | 18:29 |
| tbm | yeah, there are more corner cases than we had considered | 18:54 |
| tbm | but at least we've learned a lot about use cases | 18:54 |
| *** bkuhn is now known as bkuhnIdle | 20:21 | |
| *** bkuhnIdle is now known as bkuhn | 20:46 | |
| tgnit | tbm: yes, there are indeed! | 20:59 |
| tgnit | bkuhn: i am looking through commits in src/ , for many big changes there are already regression tests. what kind should i look for? | 21:00 |
| tgnit | tbm, johnw made sure they add enough tests for their fixes. | 21:01 |
| tbm | do ls -l under tests/* | 21:08 |
| tbm | and you'll see some empty files - size 0 | 21:08 |
| tgnit | ok | 21:09 |
| tgnit | in baseline | 21:12 |
| tgnit | opt-cleared-format | 21:13 |
| tgnit | opt-date format | 21:13 |
| tgnit | tbm: almost 14 tests | 21:14 |
Generated by irclog2html.py 2.12.1 by Marius Gedminas - find it at mg.pov.lt!