Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend the generated fixtures set and refactor AccountDeletion #6783

Closed

Conversation

Projects
None yet
2 participants
@cmrd-senya
Copy link
Member

commented Apr 5, 2016

This adds some more generation code to the fixtures builder. The AccountDeletion test is rewritten basing on the newly added fixtures. Some other tests which depended on fixtures are fixed up to conform the changes.

ref #6780

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

Well, the tests are sort of green...

Weird thing, looks like the dangling Lola broke the photo numbers feature. I can't clearly see how is it related. I would run the CI once again just in case, don't know, if it is possible though.

@cmrd-senya cmrd-senya referenced this pull request Apr 6, 2016

Merged

Account migration model/message #6750

6 of 6 tasks complete
@@ -17,8 +17,8 @@ class AccountDeleter

attr_accessor :person, :user

def initialize(diaspora_handle)
self.person = Person.where(:diaspora_handle => diaspora_handle).first
def initialize(person)

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Why? I'd rather not pass objects around workers... Why don't you use the guid here?

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Apr 21, 2016

Author Member

The change is not critical, I can revert it. But what's wrong with passing object here? It's not worker enqueueing here, so it's not passed via Redis, right?

I just thought, that in all the usages of the AccountDeleter (currently one, more in #6750) the person object is already available in some way and there is no point searching it via diaspora_handle.

For example, here person is linked via belongs_to relationship, and perhaps it is found faster than a search via diaspora_handle. But that's a pure speculation.

@@ -149,17 +149,17 @@
FactoryGirl.create(:notification, :recipient => alice, :target => @post)
FactoryGirl.create(:notification, :recipient => alice, :target => @post)

expect(Notification.where(:unread => true).count).to eq(2)
expect(Notification.where(unread: true).where(recipient: alice).count).to eq(2)

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Everywhere:

Notification.where(unread: true, recipient: alice).count
@@ -149,17 +149,17 @@
FactoryGirl.create(:notification, :recipient => alice, :target => @post)
FactoryGirl.create(:notification, :recipient => alice, :target => @post)

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

If you change to the new ruby syntax, please change the entire test.

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Apr 21, 2016

Author Member

Is it enough to change the corresponding "it" blocks? Or you mean I need to update the whole _spec.rb file?

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

I'd be fine with just changing the block. But two different syntaxes in one block look... weird :)

@@ -298,6 +298,21 @@ def r_str
end
end

factory(:report) do

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Unnecessary parenthesis.

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Actually, most of the parenthesis here are unnecessary. Please see other factory definitions...

item_type "Post"
end

factory(:role) do

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Unnecessary parenthesis.

redirect_uris %w(http://localhost:3000/)
ppid true
sector_identifier_uri "https://example.com/uri"
end

factory :o_auth_application_with_multiple_redirects, class: Api::OpenidConnect::OAuthApplication do
client_name "Diaspora Test Client"
sequence(:client_name) {|n| "Diaspora Test Client #{n}#{r_str}" }

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Why's that needed? If this is needed, you missed line 360.

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Apr 21, 2016

Author Member

If the name is the same "Diaspora Test Client" for all objects, then with my patch this test fails. It happens because this object creation depends on :o_auth_application factory and therefore we have an existing object with client_name "Diaspora Test Client" in the DB, while the test I've mentioned above makes search by the client_name and finds an unexpected object.

It seems that it is enough to make the name sequenced only for :o_auth_application factory, so I can revert name to the plain string for all the others.

@@ -14,6 +14,10 @@
describe 'fixtures' do
it 'loads fixtures' do
expect(User.count).not_to eq(0)
expect(TagFollowing.count).to be > 0

This comment has been minimized.

Copy link
@denschub

denschub Apr 21, 2016

Member

Look at the line above. Use consistent code.

@cmrd-senya cmrd-senya force-pushed the cmrd-senya:test-data-generation-refactoring branch from f1e2742 to aba4f28 Apr 22, 2016

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

Updates in this PR lead to random test suite failures due to the #6811. The issue is fixed via #6812 so I guess it should be merged and this PR then rebased to avoid the failures.

@cmrd-senya cmrd-senya force-pushed the cmrd-senya:test-data-generation-refactoring branch from aba4f28 to e63e6aa Nov 29, 2016

cmrd-senya added some commits Apr 5, 2016

Extend the generated fixtures set
This adds some more generation code to the fixtures builder. The
AccountDeletion test is rewritten basing on the newly added fixtures.
Some other tests which depended on fixtures are fixed up to conform
the changes.
Add relations to Person/User and fixup AccountDeleter
This adds has_many relations to Person and User so that Model.reflections
returns complete data and fixup AccountDeleter accordingly.

@cmrd-senya cmrd-senya force-pushed the cmrd-senya:test-data-generation-refactoring branch from e63e6aa to 314bbfa Nov 30, 2016

@cmrd-senya

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

I moved all the changes I found useful in this PR to #6726, so closing this.

@cmrd-senya cmrd-senya closed this Apr 25, 2017

@cmrd-senya cmrd-senya deleted the cmrd-senya:test-data-generation-refactoring branch Apr 25, 2017

@SuperTux88 SuperTux88 removed this from Waiting on contributor in Pull Requests Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.