Code review comment for lp:~mrazik/jenkins-launchpad-plugin/stale-locks-take2

Revision history for this message
Martin Mrazik (mrazik) wrote :

> I think a testcase is needed with a different MP in a downstream job in
> TestIsJobOrDownstreamBuildingScenarios:
> ('downstream_job_building_with_different_mp',
> {'expected': False,
> 'job_params': {'merge_proposal': 'http://mp'},
> 'jenkins_data': {
> 'builds': [{
> 'building': True,
> 'actions': [{
> 'causes': [{
> 'upstreamProject': 'job',
> 'upstreamBuild': 22
> }]
> }],
> 'result': None}],
> 'upstreamProjects': [{
> 'name': 'job',
> 'builds': [
> {
> 'number': 22,
> 'actions': [{'parameters': [{
> 'name': 'merge_proposal',
> 'value': 'http://mp3'}]}]}
> ]}]}}),
>
> The scenario is that job was started with "http://mp3" and it is still active
> in a downstream build (like generic-land). A new MP, "http://mp" is being
> checked, no matching builds should be found. I'm guessing at the correct
> scenario, it may not be quite right.

Added.

> Some comments appear to be out of place:
> - In is_job_or_downstream_building(), the docstring refers to "the json
> request below" which is actually in is_downstream_job_running().
> - is_job_or_downstream_building() and _is_item_queued_by_upstream() are
> missing parameters and return value in the docstring.
> - In _get_build_actions(), there is a comment, "# get the jenkins queue" which
> does not belong.

Fixed.

« Back to merge proposal