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.

Sign up for GitHub

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

Cds/reduce assertions per test #2563

Merged
merged 14 commits into from
Mar 6, 2023
+210 −785
Merged

Cds/reduce assertions per test #2563

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
276206d
Reduced number of assertions per test and brought Minitest/MultipleAs…
ConnorSheremeta Oct 15, 2021
a1a1eaa
System test changes
ConnorSheremeta Oct 15, 2021
666cc2b
Address review comments
ConnorSheremeta Oct 25, 2021
4ea66cf
small changes
ConnorSheremeta Oct 25, 2021
c697fbe
Merge branch 'master' into cds/reduce-assertions-per-test
ConnorSheremeta Oct 26, 2021
d43f132
Address code review feedback
ConnorSheremeta Feb 9, 2023
f08c473
# Conflicts:
ConnorSheremeta Feb 9, 2023
80fd1b7
Fix last conflict
ConnorSheremeta Feb 10, 2023
b3e7c6b
Rubocop fixes
ConnorSheremeta Feb 10, 2023
dd58c73
Merge branch 'master' into cds/reduce-assertions-per-test
ConnorSheremeta Feb 10, 2023
3fb87b1
Fix broken test which was getting another tests setup
ConnorSheremeta Feb 10, 2023
2803cb1
accidentally commited folder
ConnorSheremeta Feb 10, 2023
49c44dd
Merge branch 'master' into cds/reduce-assertions-per-test
ConnorSheremeta Mar 2, 2023
36f4556
put new test in appropriate directory
ConnorSheremeta Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,8 @@ Metrics/ParameterLists:
- app/models/jupiter_core/search.rb
- app/models/jupiter_core/solr_services/deferred_faceted_solr_query.rb

# TODO: our tests are quite smelly. We should be working to get this lower!
Minitest/MultipleAssertions:
Max: 15 # default 3
Max: 35 # default 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should aim to keep this low like 10-15 and just disable any that are exceptional until we can figure out a better way to test the thing. Basically the way it is now. I think this is important because then any new tests we write will have to keep in mind the best practice of test one thing.

Suggested change
Max: 35 # default 3
Max: 15 # default 3

Copy link
Contributor

@murny murny Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think 15 is a good default. If your over 15 expectations then you should be asking yourself why and if you can do this better.

But we shouldn't strive to get this down to 10 or 5 or.....the default of 3. There could easily be valid reasons to write tests with 10 expectations. A good integration test testing a complicated workflow would easily get close to 10 expectations and this is probably more then fine.

Think the key is keeping code readable and maintainable for the right reasons and not just cause an arbitrary linter rule is telling you to break your code into smaller chunks (which could make it far less readable or maintainable) for no real reason other then we have to keep it under a magic number. (It's the same reason why we disabled MethodLength, ClassLength, BlockLength, etc for similar reasons).

Lastly I would hate for us to write a test that is not covering the correct things because adding that one more "assertion" would violate this cop. Which should never be the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Good default, readable/maintainable code and testing the right things.

Copy link
Member

@pgwillia pgwillia Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe we can exclude the system tests. These are more likely to be testing complete workflows and have multiple detail oriented assertions.

Suggested change
Max: 35 # default 3
Max: 15 # default 3
Exclude:
- test/system/admin_users_show_test.rb
- test/system/search_test.rb


Naming/FileName:
Exclude:
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ Further discussion of the context can be found at [#2119](https://github.com/ual
- Fixture names have been modified to ensure their uniqueness [PR#2302](https://github.com/ualbertalib/jupiter/pull/2302)
- Rails upgraded to 6.0.3.7 to resolve security issues
- Added Collection and Community to reindex rake task [#2444](https://github.com/ualbertalib/jupiter/issues/2444)
- predeploy script to reference this branch
- UAT VIRTUAL_HOSTS configuration on just the containers that need it
- Refactored tests into smaller tests [PR#2563](https://github.com/ualbertalib/jupiter/pull/2563)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go up to the Unreleased section.


### Fixed
- oaisys: change etdms date source to graduation date as per LAC spec [#2298](https://github.com/ualbertalib/jupiter/pull/2510)
Expand Down
536 changes: 0 additions & 536 deletions test/integration/oaisys_list_identifiers_test.rb
View file
Open in desktop

This file was deleted.

110 changes: 0 additions & 110 deletions test/integration/oaisys_list_sets_test.rb
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion test/integration/site_for_bots_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class SiteForBotsTest < ActionDispatch::IntegrationTest
assert_select "a[rel='nofollow']", text: @item.subject.first
end

test 'structured data for google scholar' do
test 'structured thesis data for google scholar' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

# TODO: can be a lot more complicated see https://scholar.google.com/intl/en/scholar/inclusion.html#indexing
get item_path @thesis
assert_select "meta[name='citation_title'][content='#{@thesis.title}']"
Expand All @@ -125,7 +125,9 @@ class SiteForBotsTest < ActionDispatch::IntegrationTest
))
assert_select "meta[name='dc.identifier'][content='#{@item.doi}']"
assert_select "meta[name='citation_doi'][content='#{@item.doi}']"
end

test 'structured item data for google scholar' do
get item_path @item
assert_select "meta[name='citation_title'][content='#{@item.title}']"
@item.creators.each do |author|
Expand Down
6 changes: 0 additions & 6 deletions test/models/user_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ class UserTest < ActiveSupport::TestCase
assert_equal "User:#{user.id}", user.flipper_id
end

# rubocop:disable Minitest/MultipleAssertions
# TODO: our tests are quite smelly. This one needs work!
test 'should update the activity columns when not signing-in' do
pgwillia marked this conversation as resolved.
Show resolved Hide resolved
user = users(:user_regular)
assert user.last_seen_at.blank?
Expand Down Expand Up @@ -61,10 +59,7 @@ class UserTest < ActiveSupport::TestCase
assert user.previous_sign_in_ip.blank?
end
end
# rubocop:enable Minitest/MultipleAssertions

# rubocop:disable Minitest/MultipleAssertions
# TODO: our tests are quite smelly. This one needs work!
test 'should update the activity columns when signing-in' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above this could be split into three tests

  1. new user, never signed in (but don't need to write a duplicate test)
  2. updates last seen and last signed in update_activity
  3. do we need the time travel? That's just the same as the second test, right?

user = users(:user_regular)
assert user.last_seen_at.blank?
Expand Down Expand Up @@ -99,7 +94,6 @@ class UserTest < ActiveSupport::TestCase
assert_equal user.previous_sign_in_ip, ip1
end
end
# rubocop:enable Minitest/MultipleAssertions

test 'should validate if it does not have an api key and it is not a system account' do
user = User.new(
Expand Down
3 changes: 0 additions & 3 deletions test/services/doi_service_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ class DoiServiceTest < ActiveSupport::TestCase

EXAMPLE_DOI = 'doi:10.21967/fk2-ycs2-dd92'.freeze

# rubocop:disable Minitest/MultipleAssertions
# TODO: our tests are quite smelly. This one needs work!
test 'DOI state transitions' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way to break these tests up https://github.com/ualbertalib/jupiter/blob/master/test/services/datacite_doi_service_test.rb. Could go further with that making the jos and service two different tests.

But maybe just leave this as is. We'll be removing this code by December 31, 2021

@admin = users(:user_admin)

Expand Down Expand Up @@ -111,6 +109,5 @@ class DoiServiceTest < ActiveSupport::TestCase

Rails.application.secrets.doi_minting_enabled = false
end
# rubocop:enable Minitest/MultipleAssertions

end
76 changes: 37 additions & 39 deletions test/services/statistics_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

class StatisticsTest < ActiveSupport::TestCase

setup do
@obj_id = generate_random_string
@test_ip = '192.168.0.1'
end

test 'counts are zero for unviewed objects' do
obj_id = generate_random_string

Expand All @@ -10,61 +15,54 @@ class StatisticsTest < ActiveSupport::TestCase
assert_equal [0, 0], Statistics.for(item_id: obj_id)
end

# rubocop:disable Minitest/MultipleAssertions
# TODO: our tests are quite smelly. This one needs work!
test 'counts increment correctly' do
obj_id = generate_random_string
test_ip = '192.168.0.1'

test 'view counts increment correctly' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you broke this up into view and download. What do you think about breaking each of these up further?

  • testing two increments in the same time period
  • testing after expiring the key

freeze_time do
Statistics.increment_view_count_for(item_id: obj_id, ip: test_ip)
assert_equal 1, Statistics.views_for(item_id: obj_id)
assert_equal [1, 0], Statistics.for(item_id: obj_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're not exercising Statistics.for anymore?

Statistics.increment_view_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 1, Statistics.views_for(item_id: @obj_id)

# a second view inside the same time period is ignored
Statistics.increment_view_count_for(item_id: obj_id, ip: test_ip)
assert_equal 1, Statistics.views_for(item_id: obj_id)
assert_equal [1, 0], Statistics.for(item_id: obj_id)

# downloads work equally

Statistics.increment_download_count_for(item_id: obj_id, ip: test_ip)
assert_equal 1, Statistics.downloads_for(item_id: obj_id)
assert_equal [1, 1], Statistics.for(item_id: obj_id)

Statistics.increment_download_count_for(item_id: obj_id, ip: test_ip)
assert_equal 1, Statistics.downloads_for(item_id: obj_id)
assert_equal [1, 1], Statistics.for(item_id: obj_id)
Statistics.increment_view_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 1, Statistics.views_for(item_id: @obj_id)

# simulate expiring key at top of hour in order to test behaviour after ip filter rotation
views_key = Statistics.send(:uniques_key_for, :view, obj_id)
downloads_key = Statistics.send(:uniques_key_for, :download, obj_id)
views_key = Statistics.send(:uniques_key_for, :view, @obj_id)
# clear current timeout
Redis.current.persist views_key
Redis.current.persist downloads_key
# expire immediately
Redis.current.pexpire views_key, -1
Redis.current.pexpire downloads_key, -1

# Susequent viewings outside the ip filter expiry period should now count
Statistics.increment_view_count_for(item_id: obj_id, ip: test_ip)
assert_equal 2, Statistics.views_for(item_id: obj_id)
assert_equal [2, 1], Statistics.for(item_id: obj_id)
Statistics.increment_view_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 2, Statistics.views_for(item_id: @obj_id)

# a second view inside the same time period is still ignored
Statistics.increment_view_count_for(item_id: obj_id, ip: test_ip)
assert_equal 2, Statistics.views_for(item_id: obj_id)
assert_equal [2, 1], Statistics.for(item_id: obj_id)
Statistics.increment_view_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 2, Statistics.views_for(item_id: @obj_id)
end
end

test 'download counts increment correctly' do
freeze_time do
Statistics.increment_download_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 1, Statistics.downloads_for(item_id: @obj_id)

# a second view inside the same time period is ignored
Statistics.increment_download_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 1, Statistics.downloads_for(item_id: @obj_id)

# simulate expiring key at top of hour in order to test behaviour after ip filter rotation
downloads_key = Statistics.send(:uniques_key_for, :download, @obj_id)
# clear current timeout
Redis.current.persist downloads_key
# expire immediately
Redis.current.pexpire downloads_key, -1

Statistics.increment_download_count_for(item_id: obj_id, ip: test_ip)
assert_equal 2, Statistics.downloads_for(item_id: obj_id)
assert_equal [2, 2], Statistics.for(item_id: obj_id)
Statistics.increment_download_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 2, Statistics.downloads_for(item_id: @obj_id)

Statistics.increment_download_count_for(item_id: obj_id, ip: test_ip)
assert_equal 2, Statistics.downloads_for(item_id: obj_id)
assert_equal [2, 2], Statistics.for(item_id: obj_id)
Statistics.increment_download_count_for(item_id: @obj_id, ip: @test_ip)
assert_equal 2, Statistics.downloads_for(item_id: @obj_id)
end
end
# rubocop:enable Minitest/MultipleAssertions

end
12 changes: 8 additions & 4 deletions test/system/admin_communities_index_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,27 @@ class AdminCommunitiesIndexTest < ApplicationSystemTestCase
community_id: @community.id)
.save!
end
end

# TODO: add more tests
test 'should be able to expand the collection for a community in the list' do
admin = users(:user_admin)

login_user(admin)

click_link admin.name # opens user dropdown which has the admin link
click_link I18n.t('application.navbar.links.admin')
click_link I18n.t('admin.communities.index.header')
end
Comment on lines -13 to +21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart way to break this up! This is one of the problem tests. Perhaps this will reduce it's flakiness!


# TODO: add more tests
test 'ensure collections are not shown on community page' do
assert_selector 'h1', text: I18n.t('admin.communities.index.header')

# Initially, collections aren't shown, but there is a 'Collections' link
# Initially, collections aren't shown
refute_link 'Fancy Collection 0'
refute_link 'Fancy Collection 1'
refute_button 'Close'
end

test 'should be able to expand the collection for a community in the list' do
assert_selector 'a.btn', text: 'Collections'

# After clicking 'Collections', collections and close button are shown
Expand Down
3 changes: 0 additions & 3 deletions test/system/admin_users_show_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ class AdminUsersShowTest < ApplicationSystemTestCase
logout_user
end

# rubocop:disable Minitest/MultipleAssertions
# TODO: our tests are quite smelly. This one needs work!
test 'should be able to toggle suspended/admin a regular user' do
admin = users(:user_admin)
user = users(:user_regular)
Expand Down Expand Up @@ -105,7 +103,6 @@ class AdminUsersShowTest < ApplicationSystemTestCase

logout_user
end
# rubocop:enable Minitest/MultipleAssertions

test 'should be able to login as a regular user' do
admin = users(:user_admin)
Expand Down
Loading