Paul, I have pushed an update reducing the number of conditional tests to 1. Runs in O(1) constant time. Going forward, can you at least do a small favor and provide some clear code review feedback rather than something like the below? “A RunningVPPTestCase or similar should be subclassed from VPPTestcase, with your changes and in run_tests, the testcases should be downcast. I have a working prototype I could provide that I used to refactor the debug_internal code if you're interested.”
Thanks, Naveen From: <vpp-dev@lists.fd.io> on behalf of Andrew Yourtchenko <ayour...@gmail.com> Date: Tuesday, November 17, 2020 at 2:49 PM To: Paul Vinciguerra <pvi...@vinciconsulting.com> Cc: vpp-dev <vpp-dev@lists.fd.io> Subject: Re: [vpp-dev] Help clearing a -2 review. Paul, is it 2400 comparisons per single test or 2400 comparisons in total ? If the latter, I would rather optimize for readability, since it’s probably less than a second of run time. Specifically about the example with debugging of internals - replacing the hooks with subclassing hinders the intent imho - for that particular case. --a On 17 Nov 2020, at 19:50, Paul Vinciguerra <pvi...@vinciconsulting.com> wrote: Ok. This is a backwards request. I'm asking for help trying to explain properly why I've -2'd a change. The code is both useful to the community and cleanly written. I think the plumbing needs some help. When we find someone who is willing and able to contribute, I'd like to not frustrate them away. At a high level, when we run tests, the makefile sets up a specific environment that is passed to run_tests.py (which is a re-implementation of the python stdlib unittest. test runner). The test runner does discovery, that is that it finds all the tests that match a customized string, and builds a list of tests which are either run serially or forked in parallel. What people have done is put conditional logic in the test case and change the behavior after the test has started. I consider this analogous to you unrolling a loop and me coming by and testing if 1==2 for each element of your unrolled loop. The test should instead be done once in the runner, instead of 2400 or so times for every submission into the gate. To explain this, I cherry-picked some of my code and submitted it as an example. My example is here: https://gerrit.fd.io/r/c/vpp/+/29938. It' not something I planned to contribute, but I changed it enough to get it to pass the date. The commit I am blocking is here: https://gerrit.fd.io/r/c/vpp/+/29921 . How do I, with limited cycles, convey what needs to be done without writing sample code or going in and patching over someone's work. The code is well written and I'd rather +2 it and try to coax some more contributions. ;) Paul
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#18071): https://lists.fd.io/g/vpp-dev/message/18071 Mute This Topic: https://lists.fd.io/mt/78323128/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
