rospack/2008-09-04 Code Review

Package Developer: Multiple. Brian pushing through review

Reviewers:

  • Brian
  • Ken
  • Morgan
  • Rob

Code Status

  1. Code is accompanied by thorough tests

    1. Unit tests for code
      • Backquote evaluation test
        • /!\ Ken requests more complex test (done)

      • deps - check dependencies (including checking ordering and formatting everywhere)
      • deps_higher - check to make sure single-depth deps work *deps_invalid - make sure rospack fails on invalid dependency
      • depends_on - test case
      • lflags_backquote - ensure basic well-formed single lib works (redone with escaped backquoting)
      • lflags_deps - ensure flags from dependencies are properly ordered
      • lflags-deps-only - ensure that we get back ordered flags without the package's flags itself
      • sysdeps_invalid - ensure that bogus xml causes failure. Also tests that invalid xml in the system doesn't crash other tests
      • other commands on invalid xml
      • /!\ no tests for invalid manifests that are valid manifests

      • vcs0_deps - make sure empty version control is returned OK
      • vcs_deps - make sure we get version control tags from dependencies properly
      • /!\ no sysdeps0

      • sysdeps_base - make sure sysdeps works properly
      • /!\ Meaner tests requested

      • /!\ ros package path

        • /!\ multiple paths (done)

        • /!\ invalid locations (done)

      • /!\ circular dependencies (done)

      • /!\ autogenerated depth 100 list (done)

      • /!\ depends-on1 (done)

      • /!\ find (done)

      • /!\ list (done)

      • /!\ langs

      • /!\ cflags tests

      • /!\ export

      • /!\ repeated fields (name, license, etc.)

      • /!\ not setting ros-root (done)

      • /!\ invalid ros-root (done)

    2. Integration tests for ROS nodes
      • N/A
    3. Code coverage
      • Not measured. Not complete.
  2. API is stable and matches naming conventions

    • N/A
  3. All code is documented.

    • /!\ Undocumented options (predeps)

    • Generally documented
  4. Implementation review
    • /!\ Command-line parsing is spread out, hand coded, and not unified

    • /!\ Ordering of flags is currently required

    • /!\ Syntax of export would need to change to clean up parsing

Minutes

/!\ Should nag on invalid manifests /!\ need to remove rospack make from code and help

Command-line syntax and unification

  • --option vs command
  • /!\ remove "--" before foo-only-bar commands

  • export syntax is different from other commands

Possible command-line solution

  • Command, --options, and package name

Conclusion

  • Needs more robust testing
  • Needs removal of old options and change of syntax
  • Needs update of command string
  • Command-line testing passes tests, but is not extensible.


Wiki: rospack/Reviews/2008-09-04_Code_Review (last edited 2009-08-14 20:52:14 by localhost)