Both this and above session.commit would prevent from creating multiple Events or ArtifactBuilds in one transaction.

Why not just make call self.consume_git_receive_msg(get_fedmsg("git_receive_dockerfile_changed"))?

My preference is to use this format to write an assertion

self.assert*(expected_value, test_value)

Whatever the way to use, I think we need to keep it being used in all test code in freshmaker, or probably other projects we are working on. What's your opinions guys?

we'll use msg['body']['msg_id'] later to avoid call get_fedmsg twice.

create is our shortcut of adding the record and commit it, if you do that multiple times in one transation, I think that's a mis-use of this shortcut, you should just do that with the original approach of adding and commit.

I don't think we need a separate test for record_koji_build. Instead, check whether ArtifactBuild is recorded or not in above existing tests, because record_koji_build is one of the steps of each rebuild that need to verify.

For example, in test test_ensure_do_nothing_if_fail_to_login_koji, when fail to login Koji, we need to check

• nothing is build via buildContainer
• no ArtifactBuild is stored into database

It does not make sense to separate second check into another test.

How about split it into

msg = get_fedmsg("git_receive_dockerfile_changed")
self.consume_git_receive_msg(msg)

The only different between this method and record_module_build is the 4th parameter, 'image' and 'module'. Can we merge them into one method?

the original approach of adding and commit.

Do you mean we have to write another create or repeat what create does to add more Events or ArtifactBuilds at once?

On the other hand, session is passed into create via a parameter, and it gets changed, by calling commit in this case, inside the method. I don't think it's a good idea.

Same idea above.

Same idea above.

not exactly, what you need to do is:

db.session.add(...)
db.session.add(...)
db.session.commit()

create is a shortcut of add and commit, so it makes sense you should go back with add and commit in such cases instead of the shortcut. This is similar to Django's create which not requires you to save the record.

What's the benefit of this? and where consume_git_receive_msg comes from?

What's the benefit of this? and where consume_git_receive_msg comes from?

Scratch my comment about this.

Updated patches in previous version and merged them into one commit, added 2 commits to update build state in db when receives module built message and koji task state change message.

task_state is integer rather than str?

By the document, koji send the message with states in string rather than integer, I haven't check with the real message by now, datagrepper is unable to show messages under topic task.state.change (https://pagure.io/fedora-infrastructure/issue/6064), I'll capture a real message to confirm this. of course we can translate it to integer in Freshmaker, but I don't think it worth to do that since we'll need to import koji.TASK_STATE to do the translation.

I saw an example in https://fedora-fedmsg.readthedocs.io/en/latest/topics.html#buildsys-build-state-change, both new and old are integer.

We are listening on buildsys.task.state.change rather than buildsys.build.state.change: https://fedora-fedmsg.readthedocs.io/en/latest/topics.html#buildsys-task-state-change

+1

Sorry. my original comment is invalid.

+1