[ARVADOS] created: 148306afed19fc9138a04bcf161d8f24d83b2ac0
git at public.curoverse.com
git at public.curoverse.com
Mon Sep 22 14:53:36 EDT 2014
at 148306afed19fc9138a04bcf161d8f24d83b2ac0 (commit)
commit 148306afed19fc9138a04bcf161d8f24d83b2ac0
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Sep 22 14:53:13 2014 -0400
3898: Add error messages to assertions. Use assert_raise.
diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb
index 29ce969..6f9aab3 100644
--- a/services/api/test/unit/job_test.rb
+++ b/services/api/test/unit/job_test.rb
@@ -182,7 +182,7 @@ class JobTest < ActiveSupport::TestCase
test "verify job status #{parameters}" do
job = Job.create! job_attrs
assert job.valid?, job.errors.full_messages.to_s
- assert_equal job.state, 'Queued'
+ assert_equal 'Queued', job.state, "job.state"
parameters.each do |parameter|
expectations = parameter[2]
@@ -192,26 +192,23 @@ class JobTest < ActiveSupport::TestCase
if expectations.instance_of? Array
job[parameter[0]] = parameter[1]
- job.save!
+ assert_equal true, job.save, job.errors.full_messages.to_s
expectations.each do |expectation|
if expectation[1] == 'not_nil'
- assert_not_nil job[expectation[0]]
+ assert_not_nil job[expectation[0]], expectation[0]
elsif expectation[1] == 'nil'
- assert_nil job[expectation[0]]
+ assert_nil job[expectation[0]], expectation[0]
else
- assert_equal expectation[1], job[expectation[0]]
+ assert_equal expectation[1], job[expectation[0]], expectation[0]
end
end
else # String expectation, looking for error
if expectations == 'error'
- rescued = false
- begin
+ assert_raise ActiveRecord::RecordInvalid do
job[parameter[0]] = parameter[1]
+ assert job.valid?, job.errors.full_messages.to_s
job.save!
- rescue
- rescued = true
end
- assert rescued, 'Expected error'
else
raise 'I do not know how to handle this expectation'
end
commit f9a2ed53565bf921821dff5e2120b6496ac9acdd
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Sep 22 14:36:13 2014 -0400
3898: Clean up job state hooks and validations.
diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb
index 12f82be..b4aa625 100644
--- a/services/api/app/models/job.rb
+++ b/services/api/app/models/job.rb
@@ -9,11 +9,11 @@ class Job < ArvadosModel
before_create :ensure_unique_submit_id
after_commit :trigger_crunch_dispatch_if_cancelled, :on => :update
before_validation :set_priority
+ before_validation :update_timestamps_when_state_changes
+ before_validation :update_state_from_old_state_attrs
validate :ensure_script_version_is_commit
validate :find_docker_image_locator
- before_validation :verify_status
- before_create :set_state_before_save
- before_save :set_state_before_save
+ validate :validate_status
has_many :commit_ancestors, :foreign_key => :descendant, :primary_key => :script_version
@@ -242,97 +242,71 @@ class Job < ArvadosModel
end
end
- def verify_status
- changed_attributes = self.changed
+ def update_timestamps_when_state_changes
+ return if not (state_changed? or new_record?)
+ case state
+ when Running
+ self.started_at ||= Time.now
+ when Failed, Complete
+ self.finished_at ||= Time.now
+ when Cancelled
+ self.cancelled_at ||= Time.now
+ end
- if new_record?
- self.state = Queued
- elsif 'state'.in? changed_attributes
- case self.state
- when Queued
- self.running = false
- self.success = nil
- when Running
- if !self.is_locked_by_uuid
- return false
- end
- if !self.started_at
- self.started_at = Time.now
- end
- self.running = true
- self.success = nil
- when Cancelled
- if !self.cancelled_at
- self.cancelled_at = Time.now
- end
- self.running = false
- self.success = false
- when Failed
- if !self.finished_at
- self.finished_at = Time.now
- end
- self.running = false
- self.success = false
- when Complete
- if !self.finished_at
- self.finished_at = Time.now
- end
- self.running = false
- self.success = true
- end
- elsif 'cancelled_at'.in? changed_attributes
- self.state = Cancelled
+ # TODO: Remove the following case block when old "success" and
+ # "running" attrs go away. Until then, this ensures we still
+ # expose correct success/running flags to older clients, even if
+ # some new clients are writing only the new state attribute.
+ case state
+ when Queued
+ self.running = false
+ self.success = nil
+ when Running
+ self.running = true
+ self.success = nil
+ when Cancelled, Failed
self.running = false
self.success = false
- elsif 'success'.in? changed_attributes
- if self.cancelled_at
- self.state = Cancelled
- self.running = false
- self.success = false
- else
- if self.success
- self.state = Complete
- else
- self.state = Failed
- end
- if !self.finished_at
- self.finished_at = Time.now
- end
- self.running = false
- end
- elsif 'running'.in? changed_attributes
- if self.running
- self.state = Running
- if !self.started_at
- self.started_at = Time.now
- end
- else
- self.state = nil # let set_state_before_save determine what the state should be
- self.started_at = nil
- end
+ when Complete
+ self.running = false
+ self.success = true
end
+ self.running ||= false # Default to false instead of nil.
+
true
end
- def set_state_before_save
- if !self.state
- if self.cancelled_at
+ def update_state_from_old_state_attrs
+ # If a client has touched the legacy state attrs, update the
+ # "state" attr to agree with the updated values of the legacy
+ # attrs.
+ #
+ # TODO: Remove this method when old "success" and "running" attrs
+ # go away.
+ if cancelled_at_changed? or
+ success_changed? or
+ running_changed? or
+ state.nil?
+ if cancelled_at
self.state = Cancelled
- elsif self.success
- self.state = Complete
- elsif (!self.success.nil? && !self.success)
+ elsif success == false
self.state = Failed
- elsif (self.running && self.success.nil? && !self.cancelled_at)
+ elsif success == true
+ self.state = Complete
+ elsif running == true
self.state = Running
- elsif !self.started_at && !self.cancelled_at && !self.is_locked_by_uuid && self.success.nil?
+ else
self.state = Queued
end
end
-
+ true
+ end
+
+ def validate_status
if self.state.in?(States)
true
else
- errors.add :state, "'#{state.inspect} must be one of: [#{States.join ', '}]"
+ errors.add :state, "#{state.inspect} must be one of: #{States.inspect}"
false
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list