Project

General

Profile

Actions

Defect #7613

closed

Generated test instances may share the same attribute value object

Added by Etienne Massip over 13 years ago. Updated over 10 years ago.

Status:
Closed
Priority:
High
Assignee:
-
Category:
Code cleanup/refactoring
Target version:
Start date:
2011-02-12
Due date:
% Done:

0%

Estimated time:
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.


Files

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

Updated by Etienne Massip over 13 years ago

Here comes the patch.

All tests pass.

This issue was blocking me writting tests for #7456.

Actions #2

Updated by Etienne Massip over 13 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
Actions #3

Updated by Jean-Baptiste Barth over 13 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 ?

Actions #4

Updated by Etienne Massip over 13 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.

Actions #5

Updated by Jean-Baptiste Barth over 13 years ago

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

Actions #6

Updated by Etienne Massip over 13 years ago

  • Status changed from Resolved to New
Actions #7

Updated by Jean-Philippe Lang over 13 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|
Actions #8

Updated by Etienne Massip over 13 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.

Actions #9

Updated by Etienne Massip over 13 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.

Actions #10

Updated by Jean-Philippe Lang over 13 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.

Actions #11

Updated by Etienne Massip over 13 years ago

I do agree, the more clean, the better !

Actions #12

Updated by Etienne Massip over 13 years ago

  • Target version set to Candidate for next minor release
Actions #13

Updated by Etienne Massip about 13 years ago

  • Category changed from Core Plugins to Code cleanup/refactoring
Actions #14

Updated by Etienne Massip about 13 years ago

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

Still needs merging.

Actions #15

Updated by Etienne Massip about 13 years ago

  • Status changed from New to Resolved
Actions #16

Updated by Etienne Massip about 13 years ago

  • Resolution set to Fixed
Actions #17

Updated by Jean-Philippe Lang almost 13 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF