Re: [PATCH v5 2/5] Add tests for pylibfdt

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 8811 bytes --]

On Tue, Feb 21, 2017 at 09:33:01PM -0700, Simon Glass wrote:
> Hi David,
> 
> On 21 February 2017 at 19:11, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Tue, Feb 21, 2017 at 11:08:11AM -0700, Simon Glass wrote:
> >> Hi David,
> >>
> >> On 19 February 2017 at 20:42, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >> > On Thu, Feb 16, 2017 at 09:48:28PM -0700, Simon Glass wrote:
> >> >> Hi David,
> >> >>
> >> >> On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6vUpdFzICT1y@public.gmane.orgd.au> wrote:
> >> >> > On Tue, Feb 14, 2017 at 08:51:57PM -0700, Simon Glass wrote:
> >> >> >> Add a set of tests to cover the functionality in pylibfdt.
> >> >> >>
> >> >> >> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> >> >> ---
> >> >> >>
> >> >> >> Changes in v5:
> >> >> >> - Adjust tests to match new swig bindings
> >> >> >>
> >> >> >> Changes in v4:
> >> >> >> - Drop tests that are no-longer applicable
> >> >> >> - Add a get for getprop()
> >> >> >>
> >> >> >> Changes in v3:
> >> >> >> - Add some more tests
> >> >> >>
> >> >> >> Changes in v2:
> >> >> >> - Update tests for new pylibfdt
> >> >> >> - Add a few more tests to increase coverage
> >> >> >>
> >> >> >>  tests/pylibfdt_tests.py | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >>  tests/run_tests.sh      |  19 +++-
> >> >> >>  2 files changed, 285 insertions(+), 1 deletion(-)
> >> >> >>  create mode 100644 tests/pylibfdt_tests.py
> >> >> >>
> >> >>
> >> >> [..]
> >> >>
> >> >> >> +    def testPathOffset(self):
> >> >> >> +        """Check that we can find the offset of a node"""
> >> >> >> +        self.assertEquals(self.fdt.path_offset('/'), 0)
> >> >> >> +        self.assertEquals(self.fdt.path_offset('/subnode@1'), 124)
> >> >> >
> >> >> > This test is potentially fragile.  Eventually it would be nice to be
> >> >> > able to run the Python tests expecting test_tree1 on any of the copies
> >> >> > of test_tree1 we generate.  Those are required to be semantically
> >> >> > identicaly (including node/property order) to test_tree1.dtb.
> >> >> > However, some versions won't preserve exact offsets - for example
> >> >> > there's a sequence of tests where we insert additional nops in the
> >> >> > encoding to test handling of that.  That's why tests/path_offset.c,
> >> >> > for example, checks the behaviour of path_offset() against
> >> >> > subnode_offset() and knowing what property and node names are supposed
> >> >> > to be present, rather than against explicit known offsets.I'm
> >> >>
> >> >> Yes it is fragile, will check it's >0 which should be safe.
> >> >
> >> > Ok.
> >> >
> >> >> Re the tests, I feel we should try to avoid testing all the same
> >> >> things as the C code, when we could just test the interface. But it
> >> >> might be easier just to duplicate the tests you as say.
> >> >
> >> > I think so.  The set of tree1 tests is a good model, because it's
> >> > already a reasonably thorough test model of the basic libfdt
> >> > interfaces.
> >> >
> >> >> >
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> >> >> >> +            self.fdt.path_offset('/wibble')
> >> >> >> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
> >> >> >> +                          -libfdt.NOTFOUND)
> >> >> >> +
> >> >> >> +    def testPropertyOffset(self):
> >> >> >> +        """Walk through all the properties in the root node"""
> >> >> >> +        self.assertEquals(self.fdt.first_property_offset(0), ROOT_PROPS[0])
> >> >> >> +        for pos in range(len(ROOT_PROPS) - 1):
> >> >> >> +            self.assertEquals(self.fdt.next_property_offset(ROOT_PROPS[pos]),
> >> >> >> +                              ROOT_PROPS[pos + 1])
> >> >> >> +        self.assertEquals(self.fdt.next_property_offset(ROOT_PROPS[-1],
> >> >> >> +                                                        QUIET_NOTFOUND),
> >> >> >> +                          -libfdt.NOTFOUND)
> >> >> >> +
> >> >> >> +    def testPropertyOffsetExceptions(self):
> >> >> >> +        """Check that exceptions are raised as expected"""
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> >> >> >> +            self.fdt.next_property_offset(108)
> >> >> >
> >> >> > Same issue here.
> >> >>
> >> >> OK, I can just drop this one.
> >> >>
> >> >> >
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.next_property_offset(107)
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.first_property_offset(107, QUIET_NOTFOUND)
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
> >> >> >> +
> >> >> >> +        node = self.fdt.path_offset('/subnode@1/ss1')
> >> >> >> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
> >> >> >> +                          -libfdt.NOTFOUND)
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> >> >> >> +            self.fdt.first_property_offset(node)
> >> >> >> +
> >> >> >> +    def testGetName(self):
> >> >> >> +        """Check that we can get the name of a node"""
> >> >> >> +        self.assertEquals(self.fdt.get_name(0), '')
> >> >> >> +        node = self.fdt.path_offset('/subnode@1/subsubnode')
> >> >> >> +        self.assertEquals(self.fdt.get_name(node), 'subsubnode')
> >> >> >> +
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.get_name(-2)
> >> >> >> +
> >> >> >> +    def testGetPropertyByOffset(self):
> >> >> >> +        """Check that we can read the name and contents of a property"""
> >> >> >> +        root = self.fdt.path_offset('/')
> >> >> >
> >> >> > No point to this - offset of / is always 0.  If you want to test that
> >> >> > happens, make it a separate testcase.
> >> >>
> >> >> I already have it above so will drop this.
> >> >>
> >> >> >
> >> >> >> +        poffset = self.fdt.first_property_offset(root)
> >> >> >> +        prop = self.fdt.get_property_by_offset(poffset)
> >> >> >> +        self.assertEquals(prop.name, 'compatible')
> >> >> >> +        self.assertEquals(prop.value, 'test_tree1\0')
> >> >> >> +
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.get_property_by_offset(-2)
> >> >> >> +        self.assertEquals(
> >> >> >> +                -libfdt.BADOFFSET,
> >> >> >> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
> >> >> >> +
> >> >> >> +    def testGetProp(self):
> >> >> >> +        """Check that we can read the contents of a property by name"""
> >> >> >> +        root = self.fdt.path_offset('/')
> >> >> >> +        value = self.fdt.getprop(root, "compatible")
> >> >> >> +        self.assertEquals(value, 'test_tree1\0')
> >> >> >> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
> >> >> >> +                                                             QUIET_NOTFOUND))
> >> >> >
> >> >> > For testing, it might be useful to add a special value you can set the
> >> >> > quiet parameter to to make all errors quiet.
> >> >>
> >> >> Isn't that what I did?
> >> >
> >> > Sorry, I wasn't very clear.  What I mean is that if you want to treat
> >> > *all* errors as quiet, at the moment you have to do
> >> >         self.whatever(..., quiet=[NOTFOUND, EXISTS, NOSPACE,...])
> >> >
> >> > I was suggesting an extension to check_err() so you can instead say
> >> > 'quiet=SOMETHING' where SOMETHING is a special value, and it will
> >> > interpret that as making all errors quiet.
> >>
> >> OK I see. Could SOMETHING be
> >>
> >> QUIET_ALL = [NOTFOUND, EXISTS, all other errors]
> >>
> >> or are you wanting to take the dynamic typing further and use an
> >> integer or something?
> >
> > I was thinking of exploiting the dynamic typing (probably using a
> > special value, not an int, maybe 'True').  But really, either would be
> > fine.
> 
> Well I'll go with the tuple for consistency (i.e. to avoid a special
> case). It's an internal detail so we can always change if it feels
> wrong.

Yes, that makes sense.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]