Cds/reduce assertions per test #2563

ConnorSheremeta · 2021-10-15T17:24:51Z

Some of them could not be reduced as it would make the test less comprehensive.

…sertions down to 10

murny · 2021-10-18T21:11:46Z

.rubocop.yml

@@ -75,7 +75,7 @@ Metrics/ParameterLists:

 # TODO: our tests are quite smelly.  We should be working to get this lower!
 Minitest/MultipleAssertions:
-  Max: 15 # default 3
+  Max: 10 # default 3


Think this is a really bad cop and we shouldn't be striving to get this lower. It's no different with MethodLength/ABC/ClassLength etc cops. Would much prefer we turn this off or keep this a really high number instead of writing less readable code just to make this cop happy.

For example:

assert [user.last_seen_at.to_s, user.last_sign_in_at.to_s].all? now1 assert [user.last_seen_ip, user.last_sign_in_ip].all? ip1 assert [user.previous_sign_in_at, user.previous_sign_in_ip].all?(&:blank?)

is far less readable then:

assert_equal user.last_seen_at.to_s, now1 assert_equal user.last_seen_ip, ip1 assert_equal user.last_sign_in_at.to_s, now1 assert_equal user.last_sign_in_ip, ip1 assert user.previous_sign_in_at.blank? assert user.previous_sign_in_ip.blank?

There is some good work in this PR though. I think breaking up some of our massive tests into smaller, more focused tests is a good thing (like having a single test for 'structured thesis data for google scholar and one for 'structured item data for google scholar instead of one giant test).

So up to the team to decide, rather turn this cop off or leave it at a high number (like 30). Then create a PR just for refactoring these large tests into tiny ones which looks like alot of the changes in this PR is currently doing.

murny · 2021-10-18T21:15:11Z

test/system/collections_pagination_and_sort_test.rb

@@ -10,12 +10,10 @@ class CollectionsPaginationAndSortTest < ApplicationSystemTestCase
      Collection.create!(title: format("#{random_title(i)} Collection %02i", i), owner_id: admin.id,
                         community_id: @community.id)
    end
+    visit community_path(@community)


Having this in the test isn't bad thing and I would argue is more readable. Basically tests should follow 4 steps:

setup (anything that needs to happen before the action)

action (the actual thing we testing)

expectations (if the actual thing we testing meets our expectations)

cleanup (anything that needs to bring the test suite back to its previous state)

Here we moving the action (visiting the community show page) into the setup block, then the first thing in the actual tests is just expectations. This makes the code harder to read, as you now have to find what actually took place? Meaning you have to scroll to the setup block (sure its less DRY code, but thats not always a bad thing)

murny · 2021-10-18T21:20:59Z

test/system/deposit_item_test.rb

@@ -10,10 +10,6 @@ class DepositItemTest < ApplicationSystemTestCase
    click_link I18n.t('application.navbar.links.new_item')

    # 1. Describe Item Form
-
-    assert_selector 'h1', text: I18n.t('items.draft.header')
-    assert_selector 'h2', text: I18n.t('items.draft.describe_item.header')


If the only reason to remove this is to lower the number of expectations then that's a pretty bad reason 🤔

These assertions will greatly help debug these pages for future developers. By removing them, and going straight to the action of manipulating the form, then we loose confidence in these tests if something ever fails. If we cannot fill an input on the page because the input isn't on the wizard page is it because the previous wizard page failed to save? or something else happened? By asserting we on the correct wizard page before attempting to manipulate the form gives us that extra confidence that the previous wizard form worked correctly and the problem is instead of the current wizard page.

github-actions · 2021-10-25T17:00:55Z

	1 Warning
⚠️	This PR is too big! Consider breaking it down into smaller PRs.

Generated by 🚫 Danger

pgwillia

This is good stuff! I have a concern about further relaxing the Max Minitest/MultipleAssertions, a nit about the Changelog, some concerns about assertions that seem to be missing, and some suggestions.

pgwillia · 2021-10-29T16:13:38Z

CHANGELOG.md

@@ -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)


This should go up to the Unreleased section.

pgwillia · 2021-10-29T16:22:27Z

.rubocop.yml

 Minitest/MultipleAssertions:
-  Max: 15 # default 3
+  Max: 35 # default 3


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

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.

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

pgwillia · 2021-10-29T16:22:49Z

test/integration/site_for_bots_test.rb

@@ -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


test/models/user_test.rb

pgwillia · 2021-10-29T19:40:24Z

test/models/user_test.rb


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


Similar to above this could be split into three tests

new user, never signed in (but don't need to write a duplicate test)

updates last seen and last signed in update_activity

do we need the time travel? That's just the same as the second test, right?

pgwillia · 2021-10-29T20:21:35Z

test/system/admin_communities_index_test.rb

-  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


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

pgwillia · 2021-10-29T20:24:39Z

test/system/collections_pagination_and_sort_test.rb

    click_link 'Date (oldest first)'
    assert_equal URI.parse(current_url).request_uri, community_path(@community, sort: 'record_created_at',
                                                                                direction: 'asc')
    assert_selector 'button', text: 'Date (oldest first)'
    assert_selector 'div', text: '1 - 10 of 11'
    assert_selector 'ul.list-group li:first-child a', text: 'Fancy Collection 00'
    assert_selector 'ul.list-group li:nth-child(2) a', text: 'Nice Collection 01'
-    assert_selector 'ul.list-group li:nth-child(9) a', text: 'Fancy Collection 08'


Did something happen to the second last item?

pgwillia · 2021-10-29T20:25:11Z

test/system/collections_pagination_and_sort_test.rb

    click_link 'Date (newest first)'
    assert_equal URI.parse(current_url).request_uri, community_path(@community, sort: 'record_created_at',
                                                                                direction: 'desc')
    assert_selector 'button', text: 'Date (newest first)'
    assert_selector 'div', text: '1 - 10 of 11'
    assert_selector 'ul.list-group li:first-child a', text: 'Fancy Collection 10'
    assert_selector 'ul.list-group li:nth-child(2) a', text: 'Nice Collection 09'
-    assert_selector 'ul.list-group li:nth-child(9) a', text: 'Fancy Collection 02'


Why not test this?

pgwillia · 2021-10-29T20:26:41Z

test/system/collections_pagination_and_sort_test.rb

@@ -30,8 +28,10 @@ class CollectionsPaginationAndSortTest < ApplicationSystemTestCase
    assert_equal URI.parse(current_url).request_uri, community_path(@community, page: '2')
    assert_selector 'div', text: '11 - 11 of 11'
    assert_selector 'ul.list-group li:first-child a', text: 'Nice Collection 09'
+  end


Yass! This is so much better in organization, description and size of tests.

pgwillia · 2021-10-29T20:36:09Z

test/system/search_test.rb

-  # rubocop:disable Minitest/MultipleAssertions
-  # TODO: our tests are quite smelly.  This one needs work!
-  test 'anybody should be able to filter the public items' do
+  test 'anybody should be able to view search results' do


This is great. Another big improvement. Hopefully will help with the slow tests too.

# test/integration/oaisys_list_identifiers_test.rb # test/models/user_test.rb # test/services/doi_service_test.rb # test/services/statistics_test.rb

pgwillia

This is looking good! I have a question about the job test directory.

pgwillia · 2023-02-13T17:18:25Z

test/models/update_user_activity_job_test.rb

@@ -0,0 +1,20 @@
+require 'test_helper'
+
+class UpdateUserActivityJobTest < ActiveSupport::TestCase


What do you think about moving this test to test/jobs rather than test/models?

pgwillia

ConnorSheremeta added 2 commits October 15, 2021 11:06

Reduced number of assertions per test and brought Minitest/MultipleAs…

276206d

…sertions down to 10

System test changes

a1a1eaa

ConnorSheremeta requested review from lagoan, murny and pgwillia as code owners October 15, 2021 17:24

ConnorSheremeta changed the base branch from master to integration October 15, 2021 17:25

murny reviewed Oct 18, 2021

View reviewed changes

Address review comments

666cc2b

small changes

4ea66cf

ConnorSheremeta changed the base branch from integration to master October 26, 2021 15:38

Merge branch 'master' into cds/reduce-assertions-per-test

c697fbe

pgwillia requested changes Oct 29, 2021

View reviewed changes

ConnorSheremeta added 4 commits February 9, 2023 16:34

Address code review feedback

d43f132

# Conflicts:

f08c473

# test/integration/oaisys_list_identifiers_test.rb # test/models/user_test.rb # test/services/doi_service_test.rb # test/services/statistics_test.rb

Fix last conflict

80fd1b7

Rubocop fixes

b3e7c6b

ConnorSheremeta requested a review from jefferya as a code owner February 10, 2023 00:41

ConnorSheremeta added 3 commits February 10, 2023 10:39

Merge branch 'master' into cds/reduce-assertions-per-test

dd58c73

Fix broken test which was getting another tests setup

3fb87b1

accidentally commited folder

2803cb1

ConnorSheremeta requested a review from pgwillia February 10, 2023 19:48

pgwillia requested changes Feb 13, 2023

View reviewed changes

ConnorSheremeta added 2 commits March 2, 2023 09:46

Merge branch 'master' into cds/reduce-assertions-per-test

49c44dd

put new test in appropriate directory

36f4556

ConnorSheremeta requested a review from pgwillia March 2, 2023 16:49

ConnorSheremeta self-assigned this Mar 2, 2023

pgwillia approved these changes Mar 6, 2023

View reviewed changes

ConnorSheremeta merged commit c2e444a into master Mar 6, 2023

ConnorSheremeta deleted the cds/reduce-assertions-per-test branch March 6, 2023 21:57

ConnorSheremeta restored the cds/reduce-assertions-per-test branch March 7, 2023 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cds/reduce assertions per test #2563

Cds/reduce assertions per test #2563

ConnorSheremeta commented Oct 15, 2021

murny Oct 18, 2021 •

edited

Loading

murny Oct 18, 2021 •

edited

Loading

murny Oct 18, 2021 •

edited

Loading

github-actions bot commented Oct 25, 2021

pgwillia left a comment

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

murny Oct 29, 2021 •

edited

Loading

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia Oct 29, 2021

pgwillia left a comment

pgwillia Feb 13, 2023

pgwillia left a comment

		@@ -0,0 +1,20 @@
		require 'test_helper'

		class UpdateUserActivityJobTest < ActiveSupport::TestCase

Cds/reduce assertions per test #2563

Cds/reduce assertions per test #2563

Conversation

ConnorSheremeta commented Oct 15, 2021

murny Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

murny Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

murny Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2021

pgwillia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murny Oct 29, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

pgwillia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgwillia left a comment

Choose a reason for hiding this comment

murny Oct 18, 2021 •

edited

Loading

murny Oct 18, 2021 •

edited

Loading

murny Oct 18, 2021 •

edited

Loading

murny Oct 29, 2021 •

edited

Loading