Ubuntu Friendly

Merge lp://staging/~notnownikki/ubuntu-friendly/import-results-schedule-full-import into lp://staging/ubuntu-friendly

  1. import-results-schedule-full-import
  2. Merge into trunk
Proposed by Nicola Heald
Status: Merged
Merged at revision: 136
Proposed branch: lp://staging/~notnownikki/ubuntu-friendly/import-results-schedule-full-import
Merge into: lp://staging/ubuntu-friendly
Diff against target: 572 lines (+299/-82)
10 files modified
apps/releases/fixtures/initial_data.json (+11/-0)
apps/results/admin.py (+4/-0)
apps/results/api.py (+37/-35)
apps/results/management/commands/import_results.py (+32/-17)
apps/results/migrations/0005_auto__add_importschedule.py (+74/-0)
apps/results/models.py (+8/-2)
apps/results/tests.py (+132/-0)
apps/uf_util/forms.py (+0/-13)
apps/uf_util/urls.py (+0/-1)
apps/uf_util/views.py (+1/-14)
To merge this branch: bzr merge lp://staging/~notnownikki/ubuntu-friendly/import-results-schedule-full-import
Reviewer Review Type Date Requested Status
Kevin McDermott (community) Approve
Review via email: mp+102822@code.staging.launchpad.net

Description of the change

Changes the import_results procedure in the following ways:

  * Keeps track of the last time an import was run for a release
  * Uses that time to calculate the age of records that should be imported on subsequent imports
  * First import for a release does a full import and refresh of data
  * Full imports can be scheduled by setting the full_refresh flag on the schedule model
  * Now does an import for each release within one command (no more separate cron jobs)

To post a comment you must log in.
Revision history for this message
Kevin McDermott (bigkevmcd) wrote :

=== modified file 'apps/results/management/commands/import_results.py'
+ releases = Distro.objects.all()
+ now = datetime.datetime.now()

Whenever doing datetime stuff like this, utcnow is better than "now" especially when doing things like this, timezones and DST are fun ;-)

+
+ if complete_refresh:
+ # since the beginning of UF
+ delta = now - datetime.datetime(2011, 9, 1)

Maybe this should be in a constants file, or at least at the top of the file as a named value?

+
+ age = int(delta.days * 24 + delta.seconds / 60.0 / 60.0 + 1)
+
+ i = ResultsImporter(release, age)
+ i.import_results(
+ schedule,
+ complete_refresh=complete_refresh,
+ test_run_sequence=options.get('test_run_sequence') or None)

options.get('test_run_sequence') will return None, if the key doesn't exist' as options is a kwargs dict...

=== modified file 'apps/results/models.py'
--- apps/results/models.py 2011-10-28 08:30:03 +0000
+++ apps/results/models.py 2012-04-20 09:49:42 +0000
@@ -14,10 +14,16 @@
         if created:
             return lock
         return False
- except:
+ except Exception:
         return False

It's much better to be explicit in the exception, what are you expecting?

=== modified file 'apps/results/tests.py'

+class ImportResultsCommandTest(mocker.MockerTestCase, TestCase):
+
+ def setUp(self):
+ self.now = datetime.datetime(2012, 4, 19, 0, 0, 0, 0)
+ self.mocker = mocker.Mocker()

You don't need this, MockerTestCase will setup a "mocker" as self.mocker :-)

+ Distro.objects.all().delete()
+ ImportLock.objects.all().delete()

These aren't necessary...

+ self.mock_importer.import_results(
+ mocker.ARGS, complete_refresh=True, test_run_sequence=None)

mocker.ARGS should really be mocker.ANY, for a single missing (unknown) variable.

        self.mock_importer.import_results(
            mocker.ARGS, complete_refresh=False, test_run_sequence=None)

mocker.ARGS should really be schedule (there are a couple of cases of this).

I get some spurious output in the logs...

"Tests for UF are incomplete. (audio skipped)"

review: Needs Fixing
lp://staging/~notnownikki/ubuntu-friendly/import-results-schedule-full-import updated
138. By Nicola Heald

Fixes from bigkevmcd's review

Revision history for this message
Kevin McDermott (bigkevmcd) wrote :

Thanks for the fixes, +1

review: Approve
lp://staging/~notnownikki/ubuntu-friendly/import-results-schedule-full-import updated
139. By Nicola Heald

Better mocking

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches