Bug 1949126 - Improve the Rust based search engine selector tests Part 2 #7141
Conversation
mandysGit
changed the base branch from
main
to
refactor-rust-selector-tests-part-1
mandysGit
force-pushed
the
refactor-rust-selector-tests-part-2
branch
from
ef5588b to
e5ba085
Compare
Standard8
left a comment
Standard8
left a comment
There was a problem hiding this comment.
Thanks for working on this. There's a few comments in-line to address.
From the patch structure, it is pretty hard to review. The problem appears to be the moves/re-ordering of the tests within the file - this makes the differences hard to read and find. It would probably have been better to do the moves of the tests in a separate commit, and then in theory the diffs would be simpler here as it would just be the actual changes.
However, as I know there's been a lot of work in this, and it is test-only, I'm not going to ask for that here, but it would be something to bear in mind in future to make it easier for reviewers.
| thiserror = "2" | ||
| uniffi = { version = "0.29.0" } | ||
| firefox-versioning = { path = "../support/firefox-versioning" } | ||
| pretty_assertions = "1" |
There was a problem hiding this comment.
We did originally use pretty_assertions, but it was removed in #6864. I suggest discussing with the application-services-monorepo team about what the options might be.
This could be something that we remove from the patch for now, and add it in later if we get clearance.
| #[cfg(test)] | ||
| use std::collections::HashMap; | ||
|
|
||
| pub static ENGINE_BASE: Lazy<JSONEngineBase> = Lazy::new(|| JSONEngineBase { |
There was a problem hiding this comment.
I wonder that as this (and the other related ones) are in the test file now - should we name it better, e.g. JSON_ENGINE_BASE or something?
| }); | ||
|
|
||
| #[cfg(test)] | ||
| pub struct ExpectedEngineFromBase { |
There was a problem hiding this comment.
We might need a way to visually link this to ENGINE_BASE via its name, i.e. ExpectedFromJSONEngineBase or something like that.
2 participants
Pull Request checklist
[ci full]to the PR title.