Fix to hopefully resolve the flakey seed test by ensuring different ids are used #600
Conversation
spec/lib/tasks/for_education_spec.rb
Outdated
| let(:creator_id) { 'f83ba872-b16e-46e1-9f7d-df89d267550d' } | ||
| let(:teacher_id) { 'ccc9b8fd-f357-4238-983d-6f87b99bdbb2' } | ||
| let(:student_1) { 'e52de409-9210-4e94-b08c-dd11439e07d9' } # jane.smith from SeedsHelper | ||
| let(:student_2) { '0d488bec-b10d-46d3-b6f3-4cddf5d90c71' } # john.smith from SeedsHelper | ||
| let(:school_id) { 'e52de409-9210-4e94-b08c-dd11439e07d9' } # Use TEST_SCHOOL value so destroy works |
There was a problem hiding this comment.
Not sure I fully follow why changing the id fixes the flakiness?
There was a problem hiding this comment.
ah good spot...I did this ages ago, but I'm pretty sure it was another attempt to ensure the data between runs doesn't bleed
| ClimateControl.modify SEEDING_CREATOR_ID: creator_id, SEEDING_TEACHER_ID: teacher_id do | ||
| task.invoke | ||
| end |
There was a problem hiding this comment.
Does this mean these values weren't getting picked up before?
There was a problem hiding this comment.
they were getting picked up, but state was bleeding between tests
There was a problem hiding this comment.
not sure I understand how this fixes the bleed in that case 🤔
| let(:school) { School.find_by(creator_id:) } | ||
|
|
||
| before do | ||
| Rake::Task['for_education:destroy_seed_data'].invoke |
There was a problem hiding this comment.
Why is this no longer needed? Or was it never needed?
There was a problem hiding this comment.
it shouldn't have been needed, given DatabaseCleaner cleans the db between runs...but I think the concurrency was the prob...using unique ids fixes that
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the education rake task specs to improve test isolation and prevent test conflicts by using unique school IDs and properly managing environment variables.
- Introduces
ClimateControlto manage environment variables (SEEDING_CREATOR_ID,SEEDING_TEACHER_ID) in test isolation - Changes test data to use dynamically generated unique school IDs via
SecureRandom.uuidinstead of hardcoded values - Updates test IDs and improves code quality by using
.countinstead of.lengthfor ActiveRecord queries
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| before do | ||
| # Ensure the school id is unique, to avoid conflicts | ||
| stub_const('SeedsHelper::TEST_SCHOOL', school_id) | ||
| end |
There was a problem hiding this comment.
The before hook at the top level will execute for all test contexts, but school_id is defined using let which is lazily evaluated. This can cause issues with RSpec's evaluation order. Consider using let! for school_id to ensure it's evaluated before the before hook, or move the stub_const into individual test contexts where school_id is actually used.
| let(:creator_id) { 'f83ba872-b16e-46e1-9f7d-df89d267550d' } | ||
| let(:teacher_id) { 'ccc9b8fd-f357-4238-983d-6f87b99bdbb2' } |
There was a problem hiding this comment.
The hardcoded UUIDs for creator_id and teacher_id do not match the actual TEST_USERS values from SeedsHelper (which uses 583ba872-b16e-46e1-9f7d-df89d267550d for jane_doe and bbb9b8fd-f357-4238-983d-6f87b99bdbb2 for john_doe). Consider using the actual SeedsHelper::TEST_USERS values or documenting why different UUIDs are being used for testing.
| let(:creator_id) { 'f83ba872-b16e-46e1-9f7d-df89d267550d' } | |
| let(:teacher_id) { 'ccc9b8fd-f357-4238-983d-6f87b99bdbb2' } | |
| let(:creator_id) { '583ba872-b16e-46e1-9f7d-df89d267550d' } # jane_doe from SeedsHelper | |
| let(:teacher_id) { 'bbb9b8fd-f357-4238-983d-6f87b99bdbb2' } # john_doe from SeedsHelper |
Status
Ready to review
What's changed?
Fix to resolve the flakey seed test by ensuring different ids are used
Steps to perform after deploying to production
N/A