Defect #7613

Generated test instances may share the same attribute value object

Added by Etienne Massip almost 7 years ago. Updated over 3 years ago.

Status:ClosedStart date:2011-02-12
Priority:HighDue date:
Assignee:-% Done:

0%

Category:Code cleanup/refactoring
Target version:1.2.2
Resolution:Fixed Affected version:

Description

Actually, this test code will fail :

p1 = Project.generate!
p2 = Project.generate!

assert_not_equal p1.identifier, p2.identifier

That's because ObjectDaddy Project identifier generator actually returns the same object each time.

Thus, in source:trunk/test/exemplars/project_exemplar.rb :

  # Project#next_identifier is defined on Redmine
  def self.next_identifier_from_object_daddy
    @last_identifier ||= 'project-0000'
    @last_identifier.succ!
    @last_identifier
  end

Should be :

  # Project#next_identifier is defined on Redmine
  def self.next_identifier_from_object_daddy(last_identifier)
    last_identifier ||= 'project-0000'
    last_identifier.succ
  end

I'll post a patch soon.

object_daddy_exemplars.patch Magnifier (16.3 KB) Etienne Massip, 2011-02-12 14:08

Associated revisions

Revision 7458
Added by Etienne Massip about 6 years ago

Cleanup test exemplars and fix subsequent #generate calls generating same values (#7613).

Revision 7482
Added by Etienne Massip about 6 years ago

Merged r7458 from trunk (#7613).

History

#1 Updated by Etienne Massip almost 7 years ago

Here comes the patch.

All tests pass.

This issue was blocking me writting tests for #7456.

#2 Updated by Etienne Massip almost 7 years ago

  • Subject changed from Generated test instances may share the same attribute value objects to Generated test instances may share the same attribute value object

#3 Updated by Jean-Baptiste Barth almost 7 years ago

Sorry I don't really understand how your patched version works. Does ObjectDaddy pass the last_<item> as an argument implictly ?

But I see the problem in actual version. Why not a @@last_identifier instead of @last_identifier ? I think the problem is here since it's a class method, an instance variable doesn't make much sense. And it should solve your first problem.

What do you think ?

#4 Updated by Etienne Massip almost 7 years ago

Jean-Baptiste Barth wrote:

But I see the problem in actual version. Why not a @@last_identifier instead of @last_identifier ? I think the problem is here since it's a class method, an instance variable doesn't make much sense. And it should solve your first problem.

Nope, I've been a bit too much sybilline in my description :

The problem is that the generator returns the same object instance each time (@last_name), and that this instance is set by ObjectDaddy as the attribute value of the newly spawned Project instance.

That is to say :

p1.identifier.object_id == p2.identifier.object_id == @last_name.object_id 

So, doing a @last_name.succ! when generating p2 actually also change the p1.identifier value, which is not the desired effect.

If @last_name.succ! was replaced by @last_name = @last_name.succ, the test would pass, but using a class variable would not change anything.

Does ObjectDaddy pass the last_<item> as an argument implictly ?

Absolutly, it passes the previous value if generator method/block arity is 1 ; using this, IMHO, is the most elegant way to fix this issue.

#5 Updated by Jean-Baptiste Barth almost 7 years ago

Okay, I'll have a deeper look at it, thanks for the infos.

#6 Updated by Etienne Massip almost 7 years ago

  • Status changed from Resolved to New

#7 Updated by Jean-Philippe Lang over 6 years ago

I confirm the problem in current implementation but according to object_daddy documentation, there's a cleaner way to declare generators. The following patch fixes the problem for project generators:

Index: test/exemplars/project_exemplar.rb
===================================================================
--- test/exemplars/project_exemplar.rb    (revision 4892)
+++ test/exemplars/project_exemplar.rb    (working copy)
@@ -1,22 +1,9 @@
 class Project < ActiveRecord::Base
-  generator_for :name, :method => :next_name
-  generator_for :identifier, :method => :next_identifier_from_object_daddy
+  generator_for :name, :start => 'Project 0'
+  generator_for :identifier, :start => 'project-0000'
   generator_for :enabled_modules, :method => :all_modules
   generator_for :trackers, :method => :next_tracker

-  def self.next_name
-    @last_name ||= 'Project 0'
-    @last_name.succ!
-    @last_name
-  end
-
-  # Project#next_identifier is defined on Redmine
-  def self.next_identifier_from_object_daddy
-    @last_identifier ||= 'project-0000'
-    @last_identifier.succ!
-    @last_identifier
-  end
-
   def self.all_modules
     [].tap do |modules|
       Redmine::AccessControl.available_project_modules.each do |name|

#8 Updated by Etienne Massip over 6 years ago

Yes, saw that too.

I was not at ease with so much change and I thought Eric was not ignorant of this way to do when he committed the exemplars in the first place, and that he chose to write full generator methods on purpose.

But really, no idea why he didn't do that.

#9 Updated by Etienne Massip over 6 years ago

Also, there is possibility that the Issue.generate_for_project!() method in source:trunk/test/object_daddy_helpers.rb#L30 could have been written to supersede this issue, as it is called many times in a row in gantt_test.rb and that generate!() already deals with a block argument.

#10 Updated by Jean-Philippe Lang over 6 years ago

I don't think so. I think the problem hasn't been noticed before.
Anyway, I don't see any reason not to clean up these generators.

#11 Updated by Etienne Massip over 6 years ago

I do agree, the more clean, the better !

#12 Updated by Etienne Massip over 6 years ago

  • Target version set to Candidate for next minor release

#13 Updated by Etienne Massip about 6 years ago

  • Category changed from Core Plugins to Code cleanup/refactoring

#14 Updated by Etienne Massip about 6 years ago

  • Target version changed from Candidate for next minor release to 1.2.2

Still needs merging.

#15 Updated by Etienne Massip about 6 years ago

  • Status changed from New to Resolved

#16 Updated by Etienne Massip about 6 years ago

  • Resolution set to Fixed

#17 Updated by Jean-Philippe Lang about 6 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF