openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

There should be a way to mock out SQLAlchemy stuff, or just create an in-memory database. I'll see if I can find some instructions on doing that.

In the mean time, please fix up your tests as per my inline comments.

Diff comments:

> === modified file 'openlp/plugins/songs/lib/db.py'
> --- openlp/plugins/songs/lib/db.py	2014-07-05 20:04:17 +0000
> +++ openlp/plugins/songs/lib/db.py	2014-09-13 22:06:00 +0000
> @@ -31,8 +31,6 @@
>  the Songs plugin
>  """
>  
> -import re
> -
>  from sqlalchemy import Column, ForeignKey, Table, types
>  from sqlalchemy.orm import mapper, relation, reconstructor
>  from sqlalchemy.sql.expression import func, text
> @@ -329,7 +327,9 @@
>          Column('topic_id', types.Integer(), ForeignKey('topics.id'), primary_key=True)
>      )
>  
> -    mapper(Author, authors_table)
> +    mapper(Author, authors_table, properties={
> +        'songs': relation(Song, secondary=authors_songs_table, viewonly=True)
> +    })
>      mapper(AuthorSong, authors_songs_table, properties={
>          'author': relation(Author)
>      })
> 
> === modified file 'tests/functional/openlp_core_lib/test_ui.py'
> --- tests/functional/openlp_core_lib/test_ui.py	2014-06-08 14:27:03 +0000
> +++ tests/functional/openlp_core_lib/test_ui.py	2014-09-13 22:06:00 +0000
> @@ -154,6 +154,34 @@
>          self.assertEqual('my tooltip', action.toolTip())
>          self.assertEqual('my statustip', action.statusTip())
>  
> +    def test_create_action_2(self):

Please give the test a better name, and note that tests should be named ..._test not test_... e.g. create_checked_disabled_invisible_action_test()

> +        """
> +        Test creating an action

Please change the docstring to match the test, otherwise we don't know which test failed. e.g. "Test that an invisible, disabled, checked action is created correctly"

> +        """
> +        # GIVEN: A dialog
> +        dialog = QtGui.QDialog()
> +
> +        # WHEN: We create an action with some properties
> +        action = create_action(dialog, 'my_action', checked=True, enabled=False, visible=False)
> +
> +        # THEN: These properties should be set
> +        self.assertTrue(action.isChecked())

Please add an assert comment. "The action should be checked"

> +        self.assertFalse(action.isEnabled())

Please add an assert comment. "The action should be disabled"

> +        self.assertFalse(action.isVisible())

Please add an assert comment. "The action should be invisble"

> +
> +    def test_create_action_separator(self):
> +        """
> +        Test creating an action as separator
> +        """
> +        # GIVEN: A dialog
> +        dialog = QtGui.QDialog()
> +
> +        # WHEN: We create an action as a separator
> +        action = create_action(dialog, 'my_action', separator=True)
> +
> +        # THEN: The action should be a separator
> +        self.assertTrue(action.isSeparator())
> +
>      def test_create_checked_enabled_visible_action(self):
>          """
>          Test creating an action with the 'checked', 'enabled' and 'visible' properties.
> 


-- 
https://code.launchpad.net/~sam92/openlp/bug-1369139/+merge/234582
Your team OpenLP Core is subscribed to branch lp:openlp.