From 644f99e34c85f9b8d369d05b7acccb9f6b4141fc Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 8 Jun 2025 09:39:24 +0100 Subject: [PATCH 1/6] Patch administrate to avoid incorrect deprecation Previously I was seeing many instances of the following deprecation warning when running `spec/features/admin/schools_spec.rb` which was cluttering up the build output making it hard to see genuine problems: DEPRECATION WARNING: The option :class_name is deprecated. Administrate should detect it automatically. This was because when the automatic associations functionality [1] was added to administrate in v0.15.0, the `class_name` option on `Administrate::Field::Associative#associated_class_name` which is the superclass for `Administrate::Field::BelongsTo`. The latter is used on the `SchoolDashboard` with the `class_name` option [3]. However, in the v1.0.0.beta3 release of administrate [4], this deprecation was reversed [4]. So we could avoid the deprecation by upgrading administrate. However, since it's a major version bump, it's like to be a chunk of work. So instead I've patched this specific functionality to avoid the deprecation warning. The patch includes code which will fail fast once administrate is updated to v1.0.0.beta3 or later so that the patch can be removed. [1]: https://github.com/thoughtbot/administrate/pull/1633 [2]: https://github.com/thoughtbot/administrate/blob/f7287410a93d07f222bd5d4fb5055c6937eace08/CHANGELOG.md#0150-february-26-2021 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/f397e870f2a33cce1f53b9104c52314f5233572c/app/dashboards/school_dashboard.rb#L14 [4]: https://github.com/thoughtbot/administrate/pull/2697 --- config/initializers/administrate.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 config/initializers/administrate.rb diff --git a/config/initializers/administrate.rb b/config/initializers/administrate.rb new file mode 100644 index 000000000..5a49a7479 --- /dev/null +++ b/config/initializers/administrate.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'administrate/field/associative' + +module Administrate + module Field + class Associative < Base + module Overrides + def deprecated_option(name) + name == :class_name ? options.fetch(name) : super + end + end + end + end +end + +# Ref: https://github.com/thoughtbot/administrate/commit/f9c5f1af0bd27dbe8e98d43b2074b96004689ad5 +patch_no_longer_required = Gem::Version.new(Administrate::VERSION) >= Gem::Version.new('1.0.0.beta3') +raise 'Administrate::Field::Associative::Overrides patch is no longer required' if patch_no_longer_required + +Administrate::Field::Associative.prepend(Administrate::Field::Associative::Overrides) From 238ae1197d2cf15a9bd60d4afcdea88e23918ef0 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 8 Jun 2025 09:54:23 +0100 Subject: [PATCH 2/6] Add examples to fix pending example warning The `SchoolProject` was added in #490, but for some reason this spec was left with only a pending example. The latter is displayed in a warning list when you run the specs and constantlt makes me think something is wrong. To avoid this, I've implemented a couple of examples to test the most obvious behaviour of the `SchoolProject` model which seems better than nothing. These examples shouldn't be taken as an endorsement of the existing behaviour of or the desired behaviour of `SchoolProject`. For example, I wonder whether it should have some validations. --- spec/models/school_project_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/models/school_project_spec.rb b/spec/models/school_project_spec.rb index 54cc3a717..1bdcd94ee 100644 --- a/spec/models/school_project_spec.rb +++ b/spec/models/school_project_spec.rb @@ -3,5 +3,6 @@ require 'rails_helper' RSpec.describe SchoolProject do - pending "add some examples to (or delete) #{__FILE__}" + it { is_expected.to belong_to(:school) } + it { is_expected.to belong_to(:project) } end From 85b50beaa55a30bd9bb8f985027cf22bc80d1f0a Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 6 May 2025 15:09:25 +0100 Subject: [PATCH 3/6] Add invite spec example to diagnose flakey example The example just before this one keeps failing in the CircleCI build [1], but I haven't been able to replicate it locally. It's hard to work out what's going on, because `SchoolTeacher::Invite#call` swallows any exception. I'm hoping that this new assertion _might_ fail for the same reason, but then display the exception message to give us more clues. [1]: https://app.circleci.com/pipelines/github/RaspberryPiFoundation/editor-api/2359/workflows/ecf1c252-3f22-45e6-b0fe-08363f66c8d9/jobs/4447 --- spec/concepts/school_teacher/invite_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/concepts/school_teacher/invite_spec.rb b/spec/concepts/school_teacher/invite_spec.rb index 6cb579110..43439010b 100644 --- a/spec/concepts/school_teacher/invite_spec.rb +++ b/spec/concepts/school_teacher/invite_spec.rb @@ -16,6 +16,11 @@ expect(response.success?).to be(true) end + it 'does not return an error in operation response' do + response = described_class.call(school:, school_teacher_params:, token:) + expect(response[:error]).to be_blank + end + it 'creates a TeacherInvitation' do expect { described_class.call(school:, school_teacher_params:, token:) }.to change(TeacherInvitation, :count) end From 93535ccefc00ee94d3b405f9d793655a1c16ed57 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 8 Jun 2025 10:08:38 +0100 Subject: [PATCH 4/6] Configure Faker randomness with RSpec seed There are at least two flakey specs: * `spec/concepts/school_teacher/invite_spec.rb:14` [1] * `spec/concepts/project/create_remix_spec.rb:125` [2] I strongly suspect the extensive use of Faker in factories and spec setup and the randomness associated with it may be the root cause of the flakey specs. If nothing else it makes it hard to reproduce the spec failures locally. Configuring Faker's randomness with the same seed as RSpec is using and reporting should make it easier to reproduce any flakey specs in the future. [1]: https://github.com/RaspberryPiFoundation/editor-api/blob/f397e870f2a33cce1f53b9104c52314f5233572c/spec/concepts/school_teacher/invite_spec.rb#L14-L17 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/f397e870f2a33cce1f53b9104c52314f5233572c/spec/concepts/project/create_remix_spec.rb#L125-L128 --- spec/support/faker.rb | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 spec/support/faker.rb diff --git a/spec/support/faker.rb b/spec/support/faker.rb new file mode 100644 index 000000000..51d8d9cf7 --- /dev/null +++ b/spec/support/faker.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +RSpec.configure do |config| + config.before(:suite) do + puts "Faker randomized with seed #{config.seed}" + Faker::Config.random = Random.new(config.seed) + end +end From 0b3acd2091cf949e10bcda79c02df1264ec218ce Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 8 Jun 2025 10:30:34 +0100 Subject: [PATCH 5/6] Fix flakey example in Project::CreateRemix spec I've reproduced this example failure locally as follows: bundle exec rspec --seed 58786 spec/concepts/project/create_remix_spec.rb:125 The problem was with the `component` factory using `Faker::Lorem.word` to generate `Component#name` [1] and then the example relying on the order of components which is partly based on `Component#name` [2]. And thus depending on the value of the generated name, `remixed_project.components.first` in the example didn't always refer to the component that the author intended and the assertion would fail, e.g. with the seed value of 58786, the original component had the name "ad" which comes before the new component name of "added_component" alphabetically. While I have some reservations about the use of Faker in spec setup, the simplest way to fix this is to find the new component by name before asserting against its attribute values. [1]: https://github.com/RaspberryPiFoundation/editor-api/blob/f397e870f2a33cce1f53b9104c52314f5233572c/spec/factories/component.rb#L5 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/f397e870f2a33cce1f53b9104c52314f5233572c/app/models/project.rb#L14 --- spec/concepts/project/create_remix_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/concepts/project/create_remix_spec.rb b/spec/concepts/project/create_remix_spec.rb index 035cf65b9..3978c47bc 100644 --- a/spec/concepts/project/create_remix_spec.rb +++ b/spec/concepts/project/create_remix_spec.rb @@ -124,7 +124,8 @@ it 'persists the new component' do remixed_project = create_remix[:project] - expect(remixed_project.components.first.attributes.symbolize_keys).to include(new_component_params) + new_component = remixed_project.components.find { |c| c.name == new_component_params[:name] } + expect(new_component.attributes.symbolize_keys).to include(new_component_params) end end From 6beb0abfded04ab26074373828d5a7dedd3acc1f Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 8 Jun 2025 11:29:50 +0100 Subject: [PATCH 6/6] Configure CEfE generator randomness with RSpec seed We still have at least one flakey spec [1] and I noticed it depends on the `verified_school` factory which uses the `ForEducationCodeGenerator.generate` method to set the `School#code`. On the offchance this is causing some kind of non-deterministic problem and to make that generator more deterministic in specs, I thought it would be worthwhile using the RSpec seed to seed the random number generator (RNG) used by `ForEducationCodeGenerator.generate`. The changes in this commit allow the RNG to be overridden and adds file in `spec/support` to do this in specs so it uses the RSpec seed. [1]: https://github.com/RaspberryPiFoundation/editor-api/blob/f397e870f2a33cce1f53b9104c52314f5233572c/spec/concepts/school_teacher/invite_spec.rb#L14-L17 --- lib/for_education_code_generator.rb | 6 +++++- spec/lib/for_education_code_generator_spec.rb | 4 +--- spec/support/for_education_code_generator.rb | 8 ++++++++ 3 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 spec/support/for_education_code_generator.rb diff --git a/lib/for_education_code_generator.rb b/lib/for_education_code_generator.rb index 4130a01f5..f0da81148 100644 --- a/lib/for_education_code_generator.rb +++ b/lib/for_education_code_generator.rb @@ -3,8 +3,12 @@ class ForEducationCodeGenerator MAX_CODE = 1_000_000 + cattr_accessor :random + + self.random ||= Random.new + def self.generate - number = Random.new.rand(MAX_CODE) + number = random.rand(MAX_CODE) code = format('%06d', number) code.match(/(\d\d)(\d\d)(\d\d)/) do |m| diff --git a/spec/lib/for_education_code_generator_spec.rb b/spec/lib/for_education_code_generator_spec.rb index 4245cb5bc..2da0f6e1e 100644 --- a/spec/lib/for_education_code_generator_spec.rb +++ b/spec/lib/for_education_code_generator_spec.rb @@ -5,9 +5,7 @@ RSpec.describe ForEducationCodeGenerator do describe '.generate' do it 'uses Random#rand to generate a random number up to the maximum' do - random = instance_double(Random) - allow(random).to receive(:rand).with(ForEducationCodeGenerator::MAX_CODE).and_return(123) - allow(Random).to receive(:new).and_return(random) + allow(described_class.random).to receive(:rand).with(ForEducationCodeGenerator::MAX_CODE).and_return(123) expect(described_class.generate).to eq('00-01-23') end diff --git a/spec/support/for_education_code_generator.rb b/spec/support/for_education_code_generator.rb new file mode 100644 index 000000000..4f28734c3 --- /dev/null +++ b/spec/support/for_education_code_generator.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +RSpec.configure do |config| + config.before(:suite) do + puts "ForEducationCodeGenerator randomized with seed #{config.seed}" + ForEducationCodeGenerator.random = Random.new(config.seed) + end +end