Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial draft for testing practices ADR #34

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Conversation

Nitemaeric
Copy link
Contributor

@Nitemaeric Nitemaeric commented Dec 18, 2023

Context

We want to align on our testing practices to make testing both easier to write and to review.

Changes proposed in this pull request

  • Add ADR related to Testing Practices

Guidance to review

  • There's a lot of other testing practices we may want to agree on. Feel free to suggest additions.
  • Click here to preview the ADR.

Link to Trello card

https://trello.com/c/jzu6Vgel/69-write-up-adr-for-testing-practices

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Required environment variables have been updated or added to the Azure KeyVault

Screenshots

@ollietreend
Copy link
Contributor

Does this ADR proposal mean the 'Sign In as Persona' feature test will need to be converted to a system test?

https://github.com/DFE-Digital/itt-mentor-services/blob/6c36835135bf613faee30025b3c44799973f045d/spec/features/personas/sign_in_as_persona_spec.rb

@Nitemaeric
Copy link
Contributor Author

Does this ADR proposal mean the 'Sign In as Persona' feature test will need to be converted to a system test?

https://github.com/DFE-Digital/itt-mentor-services/blob/6c36835135bf613faee30025b3c44799973f045d/spec/features/personas/sign_in_as_persona_spec.rb

Yes. We would want to. There's this article I've run into that addresses these migrations: https://railsnotes.xyz/blog/feature-specs-to-system-specs

@CatalinVoineag
Copy link
Contributor

Does this ADR proposal mean the 'Sign In as Persona' feature test will need to be converted to a system test?
https://github.com/DFE-Digital/itt-mentor-services/blob/6c36835135bf613faee30025b3c44799973f045d/spec/features/personas/sign_in_as_persona_spec.rb

Yes. We would want to. There's this article I've run into that addresses these migrations: https://railsnotes.xyz/blog/feature-specs-to-system-specs

In that article, it also suggests to move to rack for running the system specs when you don't have javascript and use selenium when you have.

This might speed up our specs a bit considering we are quite light on javascript

@Nitemaeric Nitemaeric marked this pull request as ready for review December 20, 2023 12:08
Copy link
Contributor

@ollietreend ollietreend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Let's update the "Status" of this ADR to "Accepted" before merging. 👍🏻

We'll need to create Trello cards to rewrite those existing tests which don't fit this new approach.

@Nitemaeric Nitemaeric enabled auto-merge (rebase) December 21, 2023 09:43
@Nitemaeric Nitemaeric merged commit bbd2f8f into main Dec 21, 2023
4 checks passed
@Nitemaeric Nitemaeric deleted the adr/testing-practices branch December 21, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants