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

Revision history for this message
Francis Ginther (fginther) 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.

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.

Other then that, this looks good.

review: Needs Fixing

« Back to merge proposal