Skip to content

Conversation

@matthieucan
Copy link
Contributor

the ORM uses SQLAlchemy to map Firehose objects with SQL tables.

the ORM uses SQLAlchemy to map Firehose objects with SQL tables.
@matthieucan
Copy link
Contributor Author

Hi @davidmalcolm, we discussed with @paultag about integrating the orm used in firewoes directly into firehose.
Tell me what you think about this idea, and if you want things to be changed :-)

@paultag
Copy link
Contributor

paultag commented Nov 6, 2013

Thanks for this, @matthieucan !

@davidmalcolm
Copy link
Member

Thanks. Is there any automated test coverage for this? i.e. can we have, say, a tests/test_orm.py that exercises things against an in-memory sqlite db? (does this already exist within the firewoes code?)

@matthieucan
Copy link
Contributor Author

Yes, we have this in firewoes. But I need a data set, do you think the xml files in examples/ are enough or should I add those we use?

@davidmalcolm
Copy link
Member

Looking at firewoes, presumably you mean the xml files in:
https://github.com/Debian/firewoes/tree/master/tests/data , right?
I think I'd like some test cases that use the xml files in the firehose "examples" directory, they are formatted for human-readability with indentation and comments, and I know that they exhibit various features - some have optional elements, others omit optional elements etc, which is good for testing. The firewoes ones may also have these properties, but I can't easily see that since the files are each all on a single line.

That said, the firewoes examples look like real examples, and there's merit to that, and any testing is better than no testing :)

Also, is the firewoes test suite: https://github.com/Debian/firewoes/blob/master/tests/tests.py ?
That seems to exercise both the model and the controller layers (with firewoes code); for firehose itself I don't want to add a dependency on firewoes, obviously. I'd prefer a test suite that merely exercises the model layer: create a sqlite db, populate it with examples, and query it, going from XML to objects to rows and then back again to objects - if that makes sense.

Perhaps we could also have tests that simply create some objects in a loop, using logic to establish properties e.g. 10 issues, of which 3 are in one file, 7 in another, and then run a query and assert that we get the right thing back?

@matthieucan
Copy link
Contributor Author

Yes I meant these xml files in firewoes/test/data, which were more intended for firewoes anyway, those in examples/ make indeed more sense.
Yes the firewoes test suite is in firewoes/tests/tests/py, and the model/controller layers are used because I test the web app in its integrity there, especially firewoes/lib/hash.py, used to distinguish different Firehose objects, and we don't need this in Firehose.
So, using the examples/* + generated issues, all of this in a sqlite in-memory db, seems ok for me :-) I'll do this!
Thanks for the feedback!

Adds an autoincrement integer for the primary keys,
and a field 'hash' that replaces the former primary key (strings).
This permits to stay generic and still use the field "hash" in firewoes.
This script adds the analysises in examples/ to an in-memory db, queries it,
and ensures the returned values are correct.
@matthieucan
Copy link
Contributor Author

Should I add 'sqlalchemy' in requirements.txt and make travis.yml install them?

@davidmalcolm
Copy link
Member

Yes please.

@matthieucan
Copy link
Contributor Author

Since I use single table inheritance for firehose.model.{SourceRpm, DebianBinary, DebianSource}, the attribute "buildarch" exists for the three classes when they are mapped, making https://github.com/fedora-static-analysis/firehose/blob/master/tests/test_model.py#L550 fail.
I can think about two solutions:

  • separate the runs of tests/test_model.py and tests/test_orm.py, instead of using unittest discovering (which loads firehose.orm),
  • delete this assertion in test_debian_source.

What do you think?

@matthieucan
Copy link
Contributor Author

@davidmalcolm ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants