Order independent tests #4805

deivid-rodriguez · 2017-02-18T19:20:03Z

Just a WIP for now.

Fivell · 2017-02-19T14:32:22Z

lib/active_admin/resource_collection.rb

@@ -19,6 +19,10 @@ def add(resource)
      end
    end

+    def delete(resource)
+      @collection.reject! { |k| k == resource.resource_name }


@deivid-rodriguez , why not

@collection.delete(resource.resource_name )

?

Absolutely!

However, this is the only change affecting the actual code in lib and it's not really in the scope of this PR. I only added it while debugging some failures but it's not really necessary. So I'd say it's best to just remove it.

deivid-rodriguez · 2017-02-19T16:09:27Z

Ok, the specs now seem to be consistently passing with random order enabled.

Tentatively removing the WIP (until Travis proves me wrong 😄).

deivid-rodriguez · 2017-02-19T16:14:42Z

Found one more failure locally, restoring WIP.

deivid-rodriguez · 2017-02-19T16:34:03Z

Ok, it should be fixed now, let's wait for CI.

timoschilling · 2017-02-19T17:11:28Z

.travis.yml

+branches:
+  only:
+    - master
+


don't do this

Could you please be a little more educational in your reviews? I can't learn anything from "don't do this"... 😞

Anyways, it's now removed, the double CI run is now back there.

deivid-rodriguez · 2017-02-19T21:41:39Z

CI hurt, that's good. Will fix the remaining issues it surfaced as soon as possible.

Fivell · 2017-02-20T10:36:01Z

@deivid-rodriguez , looks like this almost finished, 👍 , few bugs to go

deivid-rodriguez · 2017-02-20T17:41:56Z

Did another pass. Now it's really really close to green.

deivid-rodriguez · 2017-02-20T18:02:45Z

Two more fixes pushed... This time it might pass.

deivid-rodriguez · 2017-02-20T19:46:02Z

Travis passed! Yay!

Codecov is failing but it's unrelated to this PR. My guess is that the last commit was pushed directly to master, codecov didn't generate a report for it, and now it can't find a base report to compare this against.

If that's the case, one more reason to not commit directly to master! 😉

Anyways, sorry for the length of the PR, but this was a big project. Anyone up for reviewing?

timoschilling · 2017-02-20T20:45:45Z

good job 👍

deivid-rodriguez · 2017-02-20T21:29:55Z

Pushed an extra tiny fix. Travis will have one last opportunity to surface more issues :)

Fivell · 2017-02-20T21:42:50Z

spec/unit/auto_link_spec.rb

-      end
+    it "should fallback to edit" do
+      expect(auto_link(post)).to \
+        match(%r{<a href="/admin/posts/\d+/edit">Hello World</a>})


@deivid-rodriguez , are you sure we can completely remove locale=en?

this was reasonable introduced here #4766

please take a look

Right, I accidentally removed that while refactoring. The test description was not related to locales, so I forgot to re-add it. I think those tests should be separate from the "normal case" because otherwise we don't test the "normal case". So I added be2d300, what do you think?

@deivid-rodriguez , sure

Fivell · 2017-02-20T21:58:58Z

spec/unit/auto_link_spec.rb

-      expect(self).to receive(:url_for) { |url| url }
-      expect(self).to receive(:link_to).with "Hello World", url_path
-      auto_link(post)
+      expect(auto_link(post)).to \


same as https://github.com/activeadmin/activeadmin/pull/4805/files#r102096919

Reproducible with ``` rspec \ ./spec/unit/pretty_format_spec.rb[1:7:2:2:1] \ ./spec/unit/view_helpers/display_helper_spec.rb[1:2:10] \ --seed 1234 ```

Fixes ``` rspec ./spec/unit/resource_registration_spec.rb[1:1:2] ```

Fixes ``` rspec ./spec/unit/authorization/index_overriding_spec.rb[1:1] ```

Prefer symbols.

For example ``` ./spec/unit/auto_link_spec.rb:4:in `block in <top (required)>': uninitialized constant ActiveAdmin::ViewHelpers::ActiveAdminApplicationHelper (NameError) ``` or ``` ./lib/active_admin/views/components/paginated_collection.rb:118:in `<class:PaginatedCollection>': uninitialized constant ActiveAdmin::ViewHelpers::DownloadFormatLinksHelper (NameError) ```

Fixes ``` rspec \ ./spec/unit/authorization/index_overriding_spec.rb[1:1] \ ./spec/unit/resource/routes_spec.rb[1:1:3] \ --seed 50491 ```

Fixes ``` rspec \ ./spec/unit/belongs_to_spec.rb[1:4:1] \ ./spec/unit/namespace/register_resource_spec.rb[1:5:4:1:1] \ --seed 43047 ```

They lead to leaking state very easily.

Fixes ``` rspec \ ./spec/unit/belongs_to_spec.rb[1:2:2:1] \ ./spec/unit/resource/routes_spec.rb[1:6:1:1] \ --seed 60387 ```

And properly include it.

Extend it instead. Fixes ``` rspec \ ./spec/unit/authorization/controller_authorization_spec.rb[1:2] \ ./spec/unit/views/components/index_list_spec.rb[1:1:1:1] \ --seed 42890 ```

Fixes ``` rspec \ ./spec/unit/auto_link_spec.rb[1:1:1] \ ./spec/unit/dsl_spec.rb[1:4:2] \ --seed 11315 ```

Fixes ``` rspec \ ./spec/unit/auto_link_spec.rb[1:1:1] \ ./spec/unit/resource_controller/data_access_spec.rb[1:5:1] \ --seed 12654 ```

Fixes ``` rspec \ ./spec/unit/auto_link_spec.rb[1:1:1] \ ./spec/unit/resource_controller_spec.rb[1:2:2] \ --seed 45595 ```

Fivell · 2017-02-21T18:50:16Z

@timoschilling , looks like ready to merge
@deivid-rodriguez , are you going to use parallel_specs after this ?

deivid-rodriguez · 2017-02-21T18:55:10Z

@Fivell Yes, I did that work locally but tests wouldn't pass. I'll check again after merging this, hopefully this fixes that.

timoschilling

👍

timoschilling · 2017-02-21T19:04:55Z

Good work 🎉

Fivell · 2017-02-21T19:05:45Z

@deivid-rodriguez , 🥇

deivid-rodriguez · 2017-02-21T20:26:20Z

Thanks!! :)

deivid-rodriguez mentioned this pull request Feb 19, 2017

Don't reload routes twice on admin code changes. #4558

Closed

Fivell reviewed Feb 19, 2017

View reviewed changes

deivid-rodriguez force-pushed the order_independent_tests branch from 764be57 to 91f0ec0 Compare February 19, 2017 16:06

deivid-rodriguez changed the title ~~[WIP] Order independent tests~~ Order independent tests Feb 19, 2017

deivid-rodriguez changed the title ~~Order independent tests~~ [WIP] Order independent tests Feb 19, 2017

deivid-rodriguez force-pushed the order_independent_tests branch from 91f0ec0 to 9111497 Compare February 19, 2017 16:26

deivid-rodriguez changed the title ~~[WIP] Order independent tests~~ Order independent tests Feb 19, 2017

timoschilling requested changes Feb 19, 2017

View reviewed changes

deivid-rodriguez force-pushed the order_independent_tests branch from ae1cbbe to f2e3d86 Compare February 19, 2017 17:15

deivid-rodriguez force-pushed the order_independent_tests branch 2 times, most recently from f3b0e70 to 0958ec0 Compare February 20, 2017 16:14

deivid-rodriguez force-pushed the order_independent_tests branch from 8089df6 to 08cf60d Compare February 20, 2017 21:25

Fivell reviewed Feb 21, 2017

View reviewed changes

David Rodríguez added 7 commits February 21, 2017 13:21

Make resource_localizer_spec directly runnable

7d88eb4

Move shared examples to the only file using them

ced8a16

Fix state leak in pretty_format_spec

b548fc0

Reproducible with ``` rspec \ ./spec/unit/pretty_format_spec.rb[1:7:2:2:1] \ ./spec/unit/view_helpers/display_helper_spec.rb[1:2:10] \ --seed 1234 ```

Memoize

5103e66

Make sure all tests have the necessary setup

9639cfe

Fixes ``` rspec ./spec/unit/resource_registration_spec.rb[1:1:2] ```

Typo

b3a8f60

Add missing scope

7914f34

Fixes ``` rspec ./spec/unit/authorization/index_overriding_spec.rb[1:1] ```

David Rodríguez added 20 commits February 21, 2017 13:21

Prefer load_resources to register + reload

8ac9df8

Normalize style for spec type specification

7cfc7d0

Prefer symbols.

Reload routes after configuring resource

3e78293

Fixes ``` rspec \ ./spec/unit/authorization/index_overriding_spec.rb[1:1] \ ./spec/unit/resource/routes_spec.rb[1:1:3] \ --seed 50491 ```

Ensure both resources are loaded for all specs

a7428c0

Fixes ``` rspec \ ./spec/unit/belongs_to_spec.rb[1:4:1] \ ./spec/unit/namespace/register_resource_spec.rb[1:5:4:1:1] \ --seed 43047 ```

Reset root_options every time

98361d6

Don't leak the root namespace

8d3fc87

Enable random ordering in tests by default

d232ed1

Don't use before/after(:all)

1d05eb4

They lead to leaking state very easily.

Let transactions do their job

27dd3c2

Load only what's needed for the specs to pass

47289d1

Ensure necessary resource is registered

2516871

Fixes ``` rspec \ ./spec/unit/belongs_to_spec.rb[1:2:2:1] \ ./spec/unit/resource/routes_spec.rb[1:6:1:1] \ --seed 60387 ```

Move class definition to the only file using it

581255b

Extract helper module to separate file

85065fa

And properly include it.

Don't monkeypatch ActionView::Base

788c6e9

Extend it instead. Fixes ``` rspec \ ./spec/unit/authorization/controller_authorization_spec.rb[1:2] \ ./spec/unit/views/components/index_list_spec.rb[1:1:1:1] \ --seed 42890 ```

Ensure resource controller is defined

6f74a0d

Fixes ``` rspec \ ./spec/unit/auto_link_spec.rb[1:1:1] \ ./spec/unit/dsl_spec.rb[1:4:2] \ --seed 11315 ```

Ensure necessary resource for the specs is loaded

a46c465

Fixes ``` rspec \ ./spec/unit/auto_link_spec.rb[1:1:1] \ ./spec/unit/resource_controller/data_access_spec.rb[1:5:1] \ --seed 12654 ```

Remove let variable that's always overwritten

5bce883

Move specs to the example group with correct setup

1dddda4

Fixes ``` rspec \ ./spec/unit/auto_link_spec.rb[1:1:1] \ ./spec/unit/resource_controller_spec.rb[1:2:2] \ --seed 45595 ```

Separate tests for locale in auto_link

45a51da

deivid-rodriguez force-pushed the order_independent_tests branch from be2d300 to 45a51da Compare February 21, 2017 16:23

Fivell approved these changes Feb 21, 2017

View reviewed changes

timoschilling approved these changes Feb 21, 2017

View reviewed changes

deivid-rodriguez merged commit 8166ae9 into master Feb 21, 2017

deivid-rodriguez deleted the order_independent_tests branch February 21, 2017 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order independent tests #4805

Order independent tests #4805

deivid-rodriguez commented Feb 18, 2017

Fivell Feb 19, 2017

deivid-rodriguez Feb 19, 2017

deivid-rodriguez commented Feb 19, 2017

deivid-rodriguez commented Feb 19, 2017

deivid-rodriguez commented Feb 19, 2017

timoschilling Feb 19, 2017

deivid-rodriguez Feb 19, 2017

deivid-rodriguez commented Feb 19, 2017

Fivell commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

timoschilling commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

Fivell Feb 20, 2017

deivid-rodriguez Feb 21, 2017

Fivell Feb 21, 2017

Fivell Feb 20, 2017

Fivell commented Feb 21, 2017

deivid-rodriguez commented Feb 21, 2017

timoschilling left a comment

timoschilling commented Feb 21, 2017

Fivell commented Feb 21, 2017

deivid-rodriguez commented Feb 21, 2017

Order independent tests #4805

Order independent tests #4805

Conversation

deivid-rodriguez commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Feb 19, 2017

deivid-rodriguez commented Feb 19, 2017

deivid-rodriguez commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Feb 19, 2017

Fivell commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

timoschilling commented Feb 20, 2017

deivid-rodriguez commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fivell commented Feb 21, 2017

deivid-rodriguez commented Feb 21, 2017

timoschilling left a comment

Choose a reason for hiding this comment

timoschilling commented Feb 21, 2017

Fivell commented Feb 21, 2017

deivid-rodriguez commented Feb 21, 2017