Post Fedora Automated Test Cleanup

[murny]

From #1430, I'm not able to parallelize our test suite until we refactor our current way of doing tests.

Biggest refactoring changes I see are the following:

• Remove before_all/after_all setup blocks (We should not need this in post fedora world and I'd argue we can remove the minitest-hooks gem which gives us this ability)
• Use fixtures for all data needs and turn on transactional tests. Includes cleaning up things like this:

  # Transactional tests were creating a problem where a collection defined as a
  # fixture would only be found sometimes (a race condition?)
  self.use_transactional_tests = false

More info and context from Matt's Fedora Removal PR (#1273) :

System Test Flapping
This PR also makes the flapping problem in the system tests much worse. After digging into this for a couple of days, I've realized that the system tests are actually very badly written as they currently exist. Rails expects the system tests to operate off of fixtures that are loaded once, before all of the tests run, and expects that any extra data created by the system tests will be cleaned up after the test (or at least, that subsequent tests won't care).

Existing system tests create data constantly, don't clean up after themselves, and do break if data from another test is still around. Example: This test file is a big offender. It creates 11 Communities, and then tests that those are the only 11 being given in search results -- BUT other tests also create communities, so depending on what random order tests run in there may be as many as 15 search results, which causes some assertions to fail. Conversely, if you decide to try to fix this by wiping the database between tests, These tests will fail, because fixtures only load once at the start of all system tests, and will not reload again afterwards (I spent a good deal of time trying to hack around this, to no avail). With the database wiped, there is no user record for these tests to run against.

This all (accidentally) wasn't so bad when most of the tests were based on ActiveFedora, and got wiped between tests, but with the data in ActiveRecord it becomes much more frequent that a random ordering of tests will leave stale data in a way that some other test breaks. Wiping postgresql between tests doesn't work, as it breaks the one-shot loading of fixtures that Rails does automatically, leading to different tests breaking.

The only fix here is to re-write these tests properly. As this is a pre-existing problem with the way tests are written that is just exacerbated by the different rules of ActiveRecord, that's out of scope for this PR, so I've tried to fix little bits where I could, marked a couple of the worst offenders as skip, and left the rest flapping. Fixing this with proper rewrites of these tests would be a good idea at some point.

Also my approval comment:

Second one is a heavy refactor of our test setup code. Your PR description under the "System Test Flapping" header highlights most of this. We shouldn't need the before_all/after_all blocks anymore and should be able to use fixtures heavily from now on. This will hopefully fix many of the leftover objects in database, bad cleanups, flakey tests, etc. This will also help us parallelize our tests in Rails 6.

There is also many other testing issues in the backlog which may be fixed as a result of this refactor (flakiness of test suite, etc)

[lagoan]

Do all test need to be parallel or could we parallelize a group of them ?
From some comments from my last PR, for some of my test files are being ingested programmatically to avoid dependency on active storage implementation (#1441 (comment)). I can change them back to fixtures and we would just need to modify the tests if/when they break after a change in ActiveStorage.

As for the transactional issues I would just need to take a look at the source of the problem.

[murny]

All test should work in parallel.

Don't have a full understanding of the troubles you are encountering. But fixtures do not run on the rails level. They run on the database level. They are raw SQL inserts ignoring rails models/activestorage implementations, so I don't see this being an issue? And it's also okay to use fixtures with the odd create/update via rails models in tests just make sure you are cleaning up after the tests as we do not want to pollute the test suite if that makes sense.

No need to look at the transactional issue as I think when we take on this ticket and move everything to fixtures and remove before_all setup blocks (which are really bad) this won't be an issue you are facing anymore.

Hope that explains a few things?

[lagoan]

One of the issues I had with using fixtures when testing if files existed or not is that my tests where copying files where ActiveStorage expected them to be; it is precisely because they were raw SQL inserts ignoring rails models/activestorage implementations that was the issue as the files were not handled by the ActiveStorage models.

Do you think it would be a problem for parallel tests if files are added and removed to entities created by fixtures (items, theses) as part of the cleanup? Worst case may be we have fixtures for just the tests that deal with ActiveStorage.

[murny]

Ah I think I see what your saying. When working with images/activestorage its okay to have them not in fixtures, honestly probably better if you don't.

You gotta remember fixtures will be run and setup (meaning the ENTIRE database gets cleared and recreated from fixtures) on EVERY SINGLE TEST in the code base. Hence my push back on keeping fixtures light and small in some of the PRs. For example, you don't need to create 100 users in fixtures for your tests, 2-3 user fixtures should be plenty. If you do need that amount of data, then thats a one off thing and you do it just for that specific test. Fixtures should be generic and reusable. If we are creating a new fixture JUST for one test, that should be a red flag. Either try to reuse an existing fixture, or add the code (and cleanup) to the actual test that needs it.

How to test activestorage things?

Here's hopefully a decent guide for you and other developers. For unit tests I'd create the activestorage objects right in the test. For example say we working on a review site. Let's say users can upload an image of the product with their review. I'd want to test maybe some validations around the images that can be added to a review object (e.g: file has to be of type image or size of image needs to be under a certain size). I'd write my tests as such (Note this is a real test in Rails 6 using ActionText with ActiveStorage):

  def setup
    @review = reviews(:review_one)
  end

  test 'invalid if content attachments are images' do
    blob = ActiveStorage::Blob.create_after_upload!(io: file_fixture('test.txt').open,
                                                    filename: 'test.txt',
                                                    content_type: 'text/plain')

    @review.content = ActionText::Content.new('Hello world').append_attachables(blob)

    assert_not @review.valid?
    assert_equal @review.errors[:content].first, 'attachments must be images only'
  end

  test 'invalid if content attachments are greater then 200KB' do
    blob = ActiveStorage::Blob.create_after_upload!(io: file_fixture('racecar.jpg').open,
                                                    filename: 'racecar.jpg',
                                                    content_type: 'image/jpg')

    @review.content = ActionText::Content.new('Hello world').append_attachables(blob)

    assert_not @review.valid?
    assert_equal @review.errors[:content].first, 'attachments must be under 200 KB in size'
  end
end

Only fixture I have is for the review object. Nothing for activestorage.

For controllers/system tests? Well you just want to test the more simple things like can I view an image or delete an image, etc etc.
So i'd set that up in the test themselves. Say I want to delete my profile image on a site:

  setup do
    @user = users(:user)
    sign_in @user
  end

test 'delete avatar for user' do
    @user.avatar.attach(io: file_fixture('random.jpg').open,
                        filename: 'random.jpg',
                        content_type: 'image/jpg')

    assert_difference(['ActiveStorage::Attachment.count', 'ActiveStorage::Blob.count'], -1) do
      delete settings_avatar_url
    end

    assert_redirected_to settings_profile_url
    assert_equal I18n.t('settings.avatars.destroy.avatar_removed_success'), flash[:notice]
  end
end

Again, I setup the tests by first creating the activestorage object right in my test, then I send a delete request and assert some things. Only fixture I am using is for the user object.

Where's the cleanup for files and such? Won't that pollute the test suite?
From official guides we would add something like this to our test_helper.rb:

# Cleanup activestorage
# http://edgeguides.rubyonrails.org/active_storage_overview.html#discarding-files-stored-during-integration-tests
module ActionDispatch
  class IntegrationTest
    def remove_uploaded_files
      FileUtils.rm_rf(Rails.root.join('tmp/storage'))
    end

    def after_teardown
      super
      remove_uploaded_files
    end
  end
end

[lagoan]

Thank you for your detailed explanation! This addresses my concerns and the changes to my tests when checking for files will be minimal.

[mbarnett]

Maybe to clarify a bit:

The objection to using fixtures for things like ActiveStorage::Attachments is that the table structure for that model, and the details of some of the contents like what algorithm is used to calculate the checksums, ultimately "belongs to" the Rails developers, not us, so any tests we write that make assumptions about those things (which fixtures that hardcode the structure and values, being raw SQL, by definition do) is fragile and can be broken at any time by internal changes in ActiveStorage that don't actually change its API guarantees. Essentially we're the ones doing the wrong thing by improperly building our code in a way that violates the encapsulation boundaries that other developers rely on to contain the impact of internal changes.

Hence, the attachments should be put in place by going through the proper encapsulated abstractions, so we don't make assumptions we shouldn't, in order to improve the robustness of our tests.

With respect to parallel testing, we just need to make sure that every test is going through those abstractions, and cleaning up after itself if it needs to, so that they parallelize properly.

[murny]

This is largely completed. minitest-hooks gem has been removed now. Moved a lot of code to use fixtures. Still lots of improvements can be made, but the main issues from this ticket have been resolved.

Regarding parallelization of our test suite, currently seems not to be worth the effort mainly due to Solr. More info here: #1595

Closing this issue out