On Sun, Apr 7, 2019 at 8:46 PM Cole Robinson <crobinso@xxxxxxxxxx> wrote: > > On 4/5/19 7:33 AM, Fabiano Fidêncio wrote: > > This patchset introduces a bunch of functional tests for osinfo-db-tools, > > covering all the tools and their command-lines. > > > > Daniel, what do you think we should do with regards to adding the python > > dependencies to the spec file? Just add them as BuildRequires? > > > > Yes that's how check deps are typically handled. It's a bit annoying but > there isn't really any way around it. Right. That was my understanding as well. However, there's no such thing in libvirt-dbus spec file (well, they don't have the %check target either). I'll submit a follow-up patch including the dependencies. > > > This series has not been tested on EL7 at all. So, no python2 support is > > provided unless someone has a strong opinion on adding it (then I'll find > > out what has to be done in order to properly support it). > > > > IMO just ignore py2 for now. We got this far without osinfo-db-tools > tests, older platforms can continue on just fine, let's focus on the future. > > But please drop the last patch until you've done an RPM build test on > centos/rhel7, otherwise I suspect CI is just going to break immediately > due to 1) missing BuildRequires deps for all distros and 2) lack of > conditionalizing on centos 7 where the deps aren't available. CI will break but not because of the last patch :-) As this series have been acked I'll submit the needed changes for jenkins-ci and then we have this series tested there. > > > Also, we'll need to patch libvirt-jenkins-ci in order to update the deps > > and ensure that make check is ran for osinfo-db-tools, but it's going to > > be done after we have this series reviewed. > > > > https://gitlab.com/libosinfo/osinfo-db-tools/issues/2 > > > > Fabiano Fidêncio (7): > > makefile: Add needed machinery for tests > > tests: Add util.py > > tests: Add tests data > > tests: Add osinfo-db-path tests > > tests: Add osinfo-db-validate tests > > tests: Add osinfo-db-{export,import} tests > > spec: Add %check target > > > > I didn't go over the test content with a fine tooth comb or anything but > it all looks + sounds reasonable to me, and it runs fine on my machine > and on a freebsd vm. freebsd CI will need py36-requests and a few other > python deps like we have for osinfo-db > > Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx> > > But with the last patch dropped for now as mentioned above. > > Also we should add HACKING or CONTRIBUTING docs that mention the special > environment variables for testing. We need the same for osinfo-db too Right, follow-up patches will introduce those! > > Thanks, > Cole Thanks for the review, Cole! Best Regards, -- Fabiano Fidêncio _______________________________________________ Libosinfo mailing list Libosinfo@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libosinfo
