>>> === modified file 'Makefile' >>> --- Makefile 2012-01-11 18:22:50 +0000 >>> +++ Makefile 2012-01-16 18:45:54 +0000 >>> @@ -79,7 +79,7 @@ >>> ./bin/offspring-web test offspring.web.queuemanager -s --settings=offspring.web.settings_test >>> >>> test-web-postgres: web web-test install-test-runner >>> - ./bin/offspring-web test offspring.web.queuemanager -s --settings=offspring.web.settings_devel >>> + ./bin/offspring-web test offspring.web.queuemanager --settings=offspring.web.settings_devel >>> >>> test: master slave web install-test-runner test-web master-test >>> ./.virtualenv/bin/nosetests -e 'queuemanager.*' >>> Why is this change required? >>> === modified file 'lib/offspring/web/queuemanager/models.py' >>> --- lib/offspring/web/queuemanager/models.py 2012-01-16 17:28:38 +0000 >>> +++ lib/offspring/web/queuemanager/models.py 2012-01-16 18:45:54 +0000 >>> @@ -357,24 +322,16 @@ >>> @property >>> def estimated_completion(self): >>> if self.current_job is not None: >>> - #XXX: Duplication of code in Project to get average build time >>> - cursor = connection.cursor() >>> - cursor.execute("SELECT SUM(finished_at - started_at) / count(*) from buildresults WHERE project_name = %s AND result = %s", [self.current_job.project.name, "COMPLETED"]) >>> - row = cursor.fetchone() >>> - if row[0] is not None: >>> - averageBuildTime = row[0] >>> - currentElapsedTime = datetime.now() - self.current_job.started_at >>> - estimatedTimeRemaining = averageBuildTime - currentElapsedTime >>> - #XXX: Duplication of code in Project to format output >>> - result = str(estimatedTimeRemaining) >>> - if result.startswith("-"): >>> + average_build_time = BuildResult.metrics.get_average_build_time( >>> + project=self.current_job.project) You should get the average_build_time via the average_build_time property on self.current_job.project. >>> + if average_build_time is not None: >>> + elapsed_time = (datetime.now() - self.current_job.started_at) >>> + estimated_completion =(average_build_time - >>> + elapsed_time.seconds) >>> + if estimated_completion < 0.0: >>> return "ASAP" >>> else: >>> - hours, minutes, seconds = result.split(":") >>> - if int(hours) < 1: >>> - return "%s mins, %s secs" % (minutes, math.floor(float(seconds))) >>> - else: >>> - return "%s hrs, %s mins, %s secs" % (hours, minutes, math.floor(float(seconds))) >>> + return estimated_completion >>> else: >>> return "Unknown" >>> else: >>> API wise, I think we should return a datetime object of when we expect the build to complete - 'self.current_job.started_at + datetime.timedelta(seconds=self.current_job.project.average_build_time)'. Then we can just use the Django template filter 'timeuntil' instead of writing and maintaining our own filter. >>> === modified file 'lib/offspring/web/queuemanager/tests/test_managers.py' >>> --- lib/offspring/web/queuemanager/tests/test_managers.py 2012-01-12 08:29:41 +0000 >>> +++ lib/offspring/web/queuemanager/tests/test_managers.py 2012-01-16 18:45:54 +0000 >>> @@ -43,27 +43,27 @@ >>> create_build_results_with_durations( >>> self.project, [10, 20], result=ProjectBuildStates.PENDING) >>> >>> - self.assertEqual(0.0, >>> + self.assertEqual(None, >>> BuildResult.metrics.get_average_build_time( >>> self.project)) >>> >>> def test_get_average_build_time_no_builds(self): >>> """ >>> - If there are no builds, we should get an average of 0.0. >>> + If there are no builds, we should get an average of None. >>> """ >>> - self.assertEqual(0.0, >>> + self.assertEqual(None, >>> BuildResult.metrics.get_average_build_time( >>> self.project)) >>> >>> def test_get_average_build_time_unfinished_build(self): >>> """ >>> - If a build has started, but not yet finished, this we should get 0.0 >>> + If a build has started, but not yet finished, this we should get None, >>> for the average for this entry. >>> """ This docstring needs to be reworded. "If there are no builds that have finished, we should get an average of None." >>> started_at = datetime.utcnow() >>> build_result = factory.make_build_result(project=self.project, >>> started_at=started_at) >>> - self.assertEqual(0.0, >>> + self.assertEqual(None, >>> BuildResult.metrics.get_average_build_time( >>> self.project)) >>> >>> @@ -114,22 +114,29 @@ >>> self.assertEqual(45.0 * 60, >>> BuildResult.metrics.get_average_build_time()) >>> >>> - def test_get_average_build_time_with_automated_flag(self): >>> + def test_get_average_build_time_with_automated_builds(self): I think a better name might be 'test_get_average_build_time_of_automated_builds_only'. >>> """ >>> Calling get_average_build_time with automated=True should filter the >>> results to only include automated builds, which are defined as builds >>> + without a requestor. >>> + """ >>> + create_build_results_with_durations(self.project, [10, 20]) >>> + self.assertEqual( >>> + 15 * 60.0, >>> + BuildResult.metrics.get_average_build_time(automated=True)) >>> + Instead of having magic numbers, you should calculate the average of the list you pass to create_build_results_with_durations and make 60.0 a constant. On a small tangent, I question the value of some of these tests. For example, you have a similar battery of tests for average build time as you do for average queue time. You had to make the same changes in both sets of tests because of a change you made to a function get_average_for_timeperiods that both code pathways use. Would it not be better to have tests for the behaviour of get_average_for_timeperiods / BuildResultMetricsManager._get_average_time_for_query and tests for the specific behaviours of BuildResultMetricsManager.get_average_build_time and BuildResultMetricsManager.get_average_queue_time? - that way you don't end up basically doing the same tests twice. >>> === modified file 'setup_test_database.sh' >>> --- setup_test_database.sh 2011-11-30 08:19:58 +0000 >>> +++ setup_test_database.sh 2012-01-16 18:45:54 +0000 >>> @@ -4,6 +4,7 @@ >>> CREATE ROLE offspring_test >>> CREATEROLE INHERIT LOGIN >>> PASSWORD 'temp1234' >>> + WITH CREATEDB >>> END >>> >>> sudo su postgres -c psql <>> This appears to be a syntax error. Thank you for your work on this!!