Update Spring to 3.1.1

[Hamms]

To pick up full support for Ruby 3.x, added in that version.

The primary breaking change since 2.0.1 is the removal of support for versions of Ruby older than 2.5

Links

https://github.com/rails/spring/blob/main/CHANGELOG.md#311

Testing story

Tested locally; both that this version of Spring fixes the issues we otherwise run into on Ruby 3, and that it works as expected in Ruby 2

[davidsbailey]

any insights into what this changelog entry means? not blocking, just curious since I like to use DISABLE_SPRING=1 when running bin/dashboard-server

[davidsbailey]

apologies, I'm going to attempt to answer my own question, but please feel free to correct me if I am mistaken -- it looks like this means that cache_classes may be required when running with spring 3+: rails/spring#652

this seems fine for the normal developer case, but could possibly be worth keeping in mind if you had to run locally without cache_classes for some reason, in which case you might need to disable spring?

in any case, no concerns with merging.

[Hamms]

Oh, good callout, thanks! I had glanced at that change, but only to verify that it specifically wants us to set config.cache_classes = false, which we indeed do in development:

code-dot-org/dashboard/config/environments/development.rb

Lines 4 to 7 in 25636b6

	# In the development environment your application's code is reloaded on
	# every request. This slows down response time but is perfect for development
	# since you don't have to restart the web server when you make code changes.
	config.cache_classes = false

However, I failed to realize that we're also using Spring in the test environment, and there we have the value set to true:

code-dot-org/dashboard/config/environments/test.rb

Lines 4 to 8 in 25636b6

	# The test environment is used exclusively to run your application's
	# test suite. You never need to work with it otherwise. Remember that
	# your test database is "scratch space" for the test suite and is wiped
	# and recreated between test runs. Don't rely on the data there!
	config.cache_classes = true

Tests still passed, but their documentation explicitly calls out that it's important this be false in the test environment, so I think I have some more investigation to do.

[Hamms]

Okay, after some investigation it turns out that the reason they recommend cache_classes = false in the test environment is because otherwise, Spring won't run in the test environment. So it's possible that by following their recommendation we could decrease the time it takes our tests to execute, but unfortunately doing so also causes some tests to fail.

Either way, I think the question of whether or not we want to enable the class cache in the test environment can be discussed independently of this version bump, so I'm going to set it down for now (at least, until the time comes to update Rails to 7.0).

[sureshc]

I'm still going to have to manually kill spring a few times a day, right? 😂

[Hamms]

Probably, yeah 🙃

[davidsbailey]

thanks for digging into this, @Hamms! one more question to check my understanding:

the reason they recommend cache_classes = false in the test environment is because otherwise, Spring won't run in the test environment

Can you help me understand what impact this will have on running dashboard unit tests locally -- since we are still going to have cache_classes = true in test, does this mean that bundle exec spring testunit ../path/to/my/test will stop working for developers?

[Hamms]

This PR isn't making any changes to our configuration: cache_classes will remain false in test as it has been, and we will continue to not gain any benefits from Spring in that environment until we do change that.

[davidsbailey]

@Hamms I think we may still have a problem here. would you mind double checking what I am seeing?

1. bundle exec spring testunit no longer working locally after pulling latest staging:

Dave-MBP:~/src/cdo/dashboard (staging)$ bundle exec spring testunit test/models/curriculum_reference_test.rb
/Users/dsb/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/spring-3.1.1/lib/spring/application.rb:101:in `block in preload': Spring reloads, and therefore needs the application to have reloading enabled.
Please, set config.cache_classes to false in config/environments/test.rb.

2. cache_classes appears to be true in test:

   code-dot-org/dashboard/config/environments/test.rb

   Line 8 in 36fbcfb

   config.cache_classes = true

   
   

[Hamms]

Oh, correct, I had the values backwards in my head when I was writing that comment; I meant cache_classes will remain true in test as it has been. You're right that running local tests individually with sprint testunit is no longer working, though. We could recommend developers run individual tests with ruby -Itest instead, but given that we'll have to update this eventually for Rails 7.0 anyway, perhaps we should just figure out why flipping this value breaks tests and get it working now.