It might be a good idea to also start using dedent here. That way the dummy config can be defined with the correct indentation:

from textwrap import dedent
...
def test_submit_waiver_with_arbitrary_subject_type(tmpdir):
    with patch('requests.request') as mock_request:
        ...
        p.write(dedent("""\
            [waiverdb]
            auth_method=dummy
            ...
            """))

Perhaps as a different commit?

:thumbsup:

CI currently fails with some weird ☕ exception trace.

@lholecek I've checked the logs. The step of sending CI messages to UMB hanged and was killed due to timeout.

I did test the https://pagure.io/waiverdb/pull-request/264 , but the messages were sent to dev instance of UMB. I will make some configuration change to Jenkins to switch to dev UMB again (since currently no services is picking up those messages) and rerun the build.

@lucarval yeah you are right, we use that also in Greenwave. Do you mind if I submit another PR for that? Because I should change all our tests here.

It looks like the CI ran successfully then.
Thank you @rayson!

This may give more useful errors when written as:

assert res_data <= {
    'username': 'foo',
    'subject', {'type': 'kind-of-magic', 'item': 'glibc-2.26-27.fc27'},
    ...
}
assert r.status_code == 201

@lucarval yeah you are right, we use that also in Greenwave. Do you mind if I submit another PR for that? Because I should change all our tests here.

Yup!

This may give more useful errors when written as:
assert res_data <= {
'username': 'foo',
'subject', {'type': 'kind-of-magic', 'item': 'glibc-2.26-27.fc27'},
...
}
assert r.status_code == 201

This only compares keys between dicts, not values which is really misleading!

I don't understand this comment.

Maybe: Subject type should be non empty string, actual value is: %r

https://stackoverflow.com/questions/6501583/sqlalchemy-sql-injection

Since we are storing this in the database, but we are also not checking it... someone could try sql injection attack (something like DROP TABLE etc...).. So normally I would check what in the input... but since we are using sql alchemy for the database, it should take care of it, and we don't have to be careful about it.

This only compares keys between dicts, not values which is really misleading!

Oops, I think I meant different operator or assert_*. Wasn't there something that lets you compare keys and values in dict (ignoring the missing ones)?

This only compares keys between dicts, not values which is really misleading!

Oops, I think I meant different operator or assert_*. Wasn't there something that lets you compare keys and values in dict (ignoring the missing ones)?

It's actually:

assert expected.items() <= actual.items()

Not sure if the pytest failure is more readable, e.g.

    def test_dict():
        actual = {'a': 2}
        expected = {'a': 3}
>       assert expected.items() <= actual.items()
E       AssertionError: assert dict_items([('a', 3)]) <= dict_items([('a', 2)])
E        +  where dict_items([('a', 3)]) = <built-in method items of dict object at 0x7fe297100a68>()
E        +    where <built-in method items of dict object at 0x7fe297100a68> = {'a': 3}.items
E        +  and   dict_items([('a', 2)]) = <built-in method items of dict object at 0x7fe2971008b8>()
E        +    where <built-in method items of dict object at 0x7fe2971008b8> = {'a': 2}.items

Since we are storing this in the database, but we are also not checking it... someone could try sql injection attack (something like DROP TABLE etc...).. So normally I would check what in the input... but since we are using sql alchemy for the database, it should take care of it, and we don't have to be careful about it.

It would be a big security issue if sqlalchemy wouldn't sanitize the arguments passed to queries.

I trust sqlalchemy (and the DB driver it uses) so I don't see a need for that comment.

Yeah, agreed. I just thought more comments is better than less comments :)

Yeah, agreed. I just thought more comments is better than less comments :)

No comments are the best! The code should be written in the way it's easy to understand without any comment.

The big issue is that comments are not subjected to automated test so they go stale and at some point they will contain old and no longer valid information to confuse developers.

Yeah, agreed. I just thought more comments is better than less comments :)

No comments are the best! The code should be written in the way it's easy to understand without any comment.
The big issue is that comments are not subjected to automated test so they go stale and at some point they will contain old and no longer valid information to confuse developers.

@gnaponie, yeah, let's drop the comment. I think it's adding a bit of unnecessary noise.

Rebased removing the hated comment and changed the error messages.
Anything else?

+1

:+1: