Hi Andreas, On Tue, 2017-10-24 at 17:47 +0200, Andreas Reichel wrote: > On Tue, Oct 24, 2017 at 05:23:07PM +0200, Claudius Heine wrote: > > On Tue, 2017-10-24 at 14:26 +0200, Andreas J. Reichel wrote: > > > From: Andreas Reichel <[email protected]> > > > > > > The new testcode initially provides tests for > > > * internal userspace API core functions > > > * external userspace API functions (libebgenv.a) > > > > > > They are devided into the following test-suits: > > > > > > * test_bgenv_init_retval.c, where partition probing must be > > > faked and disks must be simualted. > > > * test_probe_config_file.c, where the existence of environment > > > data is faked for simulated config partitions > > > * test_ebgenv_api_internal.c, core functions for userspace API > > > * test_ebgenv_api.c, library API functions for userspace > > > > This is a pretty large patch. Maybe split this patch up a bit? One > > patch for the empty test setup (test_main and Makefile.am), one for > > utility functions (fake_devices), and then one patch for each set > > of > > tests. > > Splitting the test code does not make it shorter.
Agree. > If a reviewer reads > 2000 lines of test-code in one file or 2000 lines of test-code in > several files... Yes? What then? Reading 2000 lines in several files is easier, because they are sorted together, hopefully logical/structural. Also multiple patches that each contain 1 header file and 1 implementation file is easier to read than 1 patch that contains multiple files because you don't have to jump around much to see how they fit together. > I don't like to have 5 patch files because the test > code has to be applied finally. If it is not okay, I will fix it. I don't understand you here. Could you elaborate? > This patch does nothing else but add the tests that are based on the > patches before. They were rewritten to fit a new test infrastructure, were they not? So its not just moving code around unchanged? If so they should be reviewed. You do know that I do this review not to anger you, but to help accelerate this process. Jan does not always has time to fully review everything, so I help you and him by doing some of it. This should normally lead to a faster patch development cycle. > > > > Not sure about the removal of the static. I see two options, one > > patch > > that removes all the 'static' or remove the 'static' within the > > patches > > that use those functions. > > > > I am quite sure. I remove the static keywoards in this patch, because > the tests need this. And the tests are introduced by this patch. > Makes > it easier to review. So you go with option 2 then. But if you split this patch up, then it might be more work for you. Just saying... Also I have some more review comments in the rest of the last mail that might have gone unnoticed. I will try to get through the rest of it at a later date. Cheers, Claudius -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: [email protected] PGP key: 6FF2 E59F 00C6 BC28 31D8 64C1 1173 CB19 9808 B153 Keyserver: hkp://pool.sks-keyservers.net -- You received this message because you are subscribed to the Google Groups "EFI Boot Guard" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/1508880186.13007.113.camel%40denx.de. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: This is a digitally signed message part
