openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

generally ok, but there's several assert statements where you have change the meaning, checking for  Falsey/Truthy values rather than checking that the the result is True, is False, is None, etc.

I've highlight a few with inline comments, but there are some others.

Diff comments:

> 
> === modified file 'tests/functional/openlp_core/common/test_init.py'
> --- tests/functional/openlp_core/common/test_init.py	2017-11-18 23:14:28 +0000
> +++ tests/functional/openlp_core/common/test_init.py	2017-12-09 16:45:47 +0000
> @@ -259,7 +258,7 @@
>          result = delete_file(None)
>  
>          # THEN: delete_file should return False
> -        self.assertFalse(result, "delete_file should return False when called with None")
> +        assert not result, "delete_file should return False when called with None"

wouldn't 'assert result is False' be better?

>  
>      def test_delete_file_path_success(self):
>          """
> @@ -272,7 +271,7 @@
>              result = delete_file(Path('path', 'file.ext'))
>  
>              # THEN: delete_file should return True
> -            self.assertTrue(result, 'delete_file should return True when it successfully deletes a file')
> +            assert result, 'delete_file should return True when it successfully deletes a file'

Same again here

>  
>      def test_delete_file_path_no_file_exists(self):
>          """
> @@ -364,4 +363,4 @@
>  
>              # THEN: log.exception should be called and get_file_encoding should return None
>              mocked_log.exception.assert_called_once_with('Error detecting file encoding')
> -            self.assertIsNone(result)
> +            assert not result

and here 'assert result is None'



-- 
https://code.launchpad.net/~trb143/openlp/asserts/+merge/335002
Your team OpenLP Core is subscribed to branch lp:openlp.