P20-829: Fix Unit.clear_cache

[artem-vavilov]

1. Fixed Unit.clear_cache: since @@script_level_cache wasn't being cleared, the data set during the first test remained unchanged in subsequent tests.
   

   code-dot-org/dashboard/app/models/unit.rb

   Lines 452 to 459 in 751e852

   	def self.script_level_cache
   	return nil unless should_cache?
   	@@script_level_cache ||= {}.tap do |cache|
   	script_cache.each_value do |unit|
   	cache.merge!(unit.script_levels.index_by(&:id))
   	end
   	end
   	end

2. Re-introduced the seed_deprecated_unit_fixtures optimization that was reverted here: P20-829: Undo optimization of deprecated unit fixtures seeding #57772

Links

• JIRA ticket P20-829
• Related PR: P20-829: Prepare Units only when needed on the Dashboard test run #57743

[artem-vavilov]

Since @@script_level_cache wasn't being cleared, the data set during the first test remained unchanged in subsequent tests:

code-dot-org/dashboard/app/models/unit.rb

Lines 452 to 459 in 751e852

	def self.script_level_cache
	return nil unless should_cache?
	@@script_level_cache ||= {}.tap do |cache|
	script_cache.each_value do |unit|
	cache.merge!(unit.script_levels.index_by(&:id))
	end
	end
	end

[artem-vavilov]

It worked previously because we seeded deprecated unit fixtures before enabling Unit caching with Unit.stubs(:should_cache?).returns(true):

code-dot-org/dashboard/test/integration/callouts_test.rb

Lines 3 to 18 in 751e852

	class CalloutsTest < ActionDispatch::IntegrationTest
	setup do
	Unit.stubs(:should_cache?).returns true
	Rails.application.config.stubs(:levelbuilder_mode).returns false
	@script = create :script
	@lesson_group = create :lesson_group, script: @script
	@lesson = create :lesson, script: @script, lesson_group: @lesson_group
	@maze_data = {game_id: 25, user_id: 1, name: '__bob4', level_num: 'custom', skin: 'birds', short_instructions: 'sdfdfs'}
	@level = Maze.create(@maze_data)
	@level.callout_json = '[{"localization_key": "run", "element_id": "#runButton"}]'
	@level.save!
	@script_level = create(:script_level, levels: [@level], lesson: @lesson, script: @script)
	@level_path = "/levels/#{@level.id}"
	@script_level_path = "/s/#{@script.name}/lessons/1/levels/1"
	Unit.script_cache.delete @script.name
	Unit.script_cache.delete @script.id.to_s

[artem-vavilov]

The error can be reproduced by running the command: cd dashboard && RAILS_ENV=test RACK_ENV=test bundle exec rails test --seed 37263 test/integration/callouts_test.rb test/models/unit_test.rb

[Hamms]

Fantastic find! Obscure interactions between two disparate unit tests can be real tough to track down; I'm so thrilled that we now know what the underlying issue was. Thank you for the investigation!

This does raise the question of why clear_cache was previously not clearing the ScriptLevel cache; glancing through the history of the method, however, it does seem like this was not an intentional exclusion. Looks like the method originally only cleared @@unit_cache (technically, @@script_cache), and the other cache values were added individually over time on an as-needed basis. That then raises a second question for me: are there any other locally cached values that should also be added to clear_cache?

I think that question can be addressed in a follow-up PR, though; this one looks great to me! Thank you for the diligent debugging.