Various improvements to the specs #561
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
floehopper
force-pushed
the
improve-specs
branch
2 times, most recently
from
245260f to
e5c2fda
Compare
raspberrypiherokubot
temporarily deployed
to
editor-api-p-improve-sp-qt35g5
chrisroos
previously approved these changes
Jun 9, 2025
There was a problem hiding this comment.
Thanks, @floehopper! I've not tested these locally but based on the commit notes and a read through the code I think they all look like great improvements 👍
|
I think one slight downside of the final commit is it makes the build output noisier again, because the specs seem to trigger/rely on some exceptions being handled. However, I think we should probably aim to eliminate or at least check these are not surprising exceptions. And thus noisier build output in the short term is probably a price worth paying. |
raspberrypiherokubot
temporarily deployed
to
editor-api-p-improve-sp-tjgax8
|
We require contributors to sign our Contributor License Agreement, and we don't have you on file. In order for us to review and merge your code, please complete this form and we'll get you added and review your contribution as soon as possible. |
raspberrypiherokubot
temporarily deployed
to
editor-api-p-improve-sp-tjgax8
floehopper
force-pushed
the
improve-specs
branch
from
60bc443 to
2c200e1
Compare
I've moved this final commit into #563, because I think it might be sensible to do some work to investigate the exceptions that are being displayed before merging this change. |
raspberrypiherokubot
temporarily deployed
to
editor-api-p-display-ca-4dzpmr
floehopper
force-pushed
the
improve-specs
branch
from
2c200e1 to
e040a30
Compare
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]: thoughtbot/administrate#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]: thoughtbot/administrate#2697
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.
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
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
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
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
floehopper
force-pushed
the
improve-specs
branch
from
e040a30 to
6beb0ab
Compare
chrisroos
approved these changes
Jun 18, 2025
There was a problem hiding this comment.
I've been through the commits again after @floehopper's most recent changes and am happy to approve. Thanks, James!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've made a series of improvements to the specs and build output as follows:
Reduce noise in build output
Make specs more deterministic
Diagnose & fix flakey example
Make it easier to diagnose spec failuresOverride Sentry.capture_exception in specs- I've moved this change into a separate PR.