Fixtures allow for push-style cascading dependency resolution with
ability to override some of the dependent fixture's arguments when needed.
This is very nice as it gives a lot of flexibility and suitable in a lot
of case. So, single fixture based approach can be applied to most of the
tests.
In addition, this result in good reusability of test code.
If you feel overwelmed and confused with complex argument list of test
functions try to read links from "Documentation" section (see below).
But here is the primer.
#[fixture]
pub fn configuration(
base_uri: &str,
base_uri_onion: &str,
) -> (TempDir, :🚀:figment::Figment) {
// ...
}
Attribute #[fixture] turn function into fixture which means now it can
be injected into tests or other fixtures by "configuration" argument name.
"base_uri", "base_uri_onion" are other fixtures which are injected into
"configuration".
#[fixture]
pub fn rocket(
#[from(configuration)] (tmpdir, config): (TempDir, :🚀:figment::Figment),
) -> (TempDir, :🚀:Rocket<:🚀:Build>) {
// ...
}
As we destructuring results of "configuration" fixture injection into
"rocket" fixture the name of the fixture can't be inferenced from
parameters name, so we have to give a hint with #[from(configuration)]
which fixture has to be called.
#[rstest]
fn hkp_add_two(
base_uri: &str,
#[from(cert)] (_, tpk_0): (&str, Cert),
#[with(alt_cert_name())]
#[from(cert)]
(_, tpk_1): (&str, Cert),
#[from(client)] (tmpdir, client): (TempDir, Client),
) {
// ...
}
This is probably the most trickies function signature, but let brake it
down.
There are 4 fixtures injected into "hkp_add_two" test function:
- base_uri
- cert
- cert
- client
I think at this point of explanation syntax for "base_uri" and "client"
fixturex should be clear.
So,
#[from(cert)] (_, tpk_0): (&str, Cert),
#[with(alt_cert_name())] #[from(cert)] (_, tpk_1): (&str, Cert),
calls to "cert" fixture which takes "cert_name" argument. Here is the
definition:
#[fixture]
pub fn cert<'a>(cert_name: &'a str) -> (&'a str, Cert) {
// ...
}
For the "hkp_add_two" test we need two certificates, so first one is
generated with default name which is returned by "cert_name" fixture,
but for the second with #[with(alt_cert_name())] we say that we want to
use "alt_cert_name" fixture for cert_name argument, so the certificate
is generated with "bar@invalid.example.com" name.
And parenthis for destructing of the results of injection as "cert"
fixture returns tuple of (cert_name, cert).
Documentation:
- https://crates.io/crates/rstest
- https://docs.rs/rstest/0.26.1/rstest/attr.fixture.html
NOTE: probably this changes made translation generation in tests more unstable.
It was already unstable, but it looks like the use of fixtures make a
bit more unstable. So, in case of test failures, just run them several
times. I'll take a look and modules compilation order later once move
tests into modules with SUT's functionality.
Changes:
- Turn the following helper functions from hagrid::web::tests
module to fixtures:
- configuration()
- rocket()
- client()
- Panic immediately in the fixtures and don't bubble up the error like
it was in helper functions. Bubbling up errors does not make sense as
we panic with expect later in tests code. So, we duplicated code
(.unwrap()) calls) and make return signatures more complex.
- Inject "base_uri" and "base_uri_onion" into "configuration" fixture as
after turning it into fixture we can do that.
- Remove BASE_URI_ONION constant as it is not used. Now "base_uri_onion"
fixture is used instead.
- Refactor all the tests which used mentioned above helper functions to
use fixture.
Changes:
- Turn "build_cert" funtion into fixture.
- Refactor all tests which relied on build_cert() to inject one or two
certs.
- Replace usage of duplicated e-mail (cert_name) in tests with value
returned by "cert" fixture (i.e. cert_name). This better align actual
cert and cert_name usage in tests and eliminate hardcoded e-mail
string slices.
Changes:
- Create fixtures based on constants:
- base_uri
- base_uri_onion
- Change tests which use BASE_URI or BASE_URI_ONION from #[test]
declaration to #[rstest]. That allows to inject fixtures.
- Replace usage of constants BASE_URI and BASE_URI_ONION with base_uri
and base_uri_onion fixtures.
There are still some usages of the constants which will be refactored
later.
- Propagate injected fixture values into check_* assertions to replace
constant usages.
Command: `just check`
Changes:
- The following warnings were fixed:
warning: hiding a lifetime that's elided elsewhere is confusing
--> database/src/sqlite.rs:146:11
|
146 | fn tx(&self) -> &Transaction {
| ^^^^^ ------------
| | ||
| | |the same lifetime is hidden here
| | the same lifetime is elided here
| the lifetime is elided here
|
= help: the same lifetime is referred to in inconsistent ways, making the signature confusing
= note: `#[warn(mismatched_lifetime_syntaxes)]` on by default
help: use `'_` for type paths
|
146 | fn tx(&self) -> &Transaction<'_> {
| ++++
warning: hiding a lifetime that's elided elsewhere is confusing
--> src/dump.rs:22:30
|
22 | pub fn display_sensitive(&self) -> SessionKeyDisplay {
| ^^^^^ ----------------- the same lifetime is hidden here
| |
| the lifetime is elided here
|
= help: the same lifetime is referred to in inconsistent ways, making the signature confusing
= note: `#[warn(mismatched_lifetime_syntaxes)]` on by default
help: use `'_` for type paths
|
22 | pub fn display_sensitive(&self) -> SessionKeyDisplay<'_> {
| ++++
If you don't have toolchain installed and you use rustup run:
$ rustup install --component rustfmt --component clippy 1.89
NOTE: It might be that you have 1.89.0 installed as stable toolchain, in
that case you still have to run the above command to install exactly
1.89.
Changes:
- Upgrade version of used toolchain in the following places:
- .gitlab-ci.yml
- Cargo.toml
- clippy.toml
- rust-toolchain.toml
Continue to dissolve hagrid::web module which had way too much responsibilities
and became the god-module (see https://en.wikipedia.org/wiki/God_object).
In addition, solved and refined several smaller issues and made
improvements.
Changes:
- Extract configuration loading (i.e. extraction) from
hagrid::web::rocket_factory() to hagrid::config::load() and call it
from hagrid::app::configure_rocket().
- Extract and split hagrid::web::run() (which previously was #[launch] on
rocket() fn) to hagrid::app::run(), hagrid::app::configure_rocket()
and hagrid::app::run_rocket().
- Remove String copy in Configuration::base_uri_onion() and return
references to internal fields of the struct. Callers can copy it when
they need to, otherwise they can just use references without overhead.
- Extract rocket() helper function from client() in tests of hagrid::web
module to separate rocket instance creation.
- Remove code duplication in tests of hagrid::web module by reusing
rocket() and client() functions for instances construction.
- Introduce idea of initializers and "hagrid::initializers" module to bootstrap
rocket framework and register states (with rocket.manage()), fairings (with
rocket.attach()) and routes (with rocket.mount()) on rocket instance.
A single initializer resides in submodule of hagrid::initializers and
register itself in hagrid::initializers::run() function (has to be
done manually by the programmer who adds new initalizer).
Initializer consist of the following two functions with some vary signatures:
- init(config: &Configuration) -> hagrid::initalizers::Return<CONSTRUCTED_TYPE>
- register(rocket: Rocket<Build>, config: &Configuration, state_fairing_or_routes: <CONSTRUCTED_TYPE>) -> Rocket<Build>
init(config: &Configuration)
The function instantiate state, fairing or routes possibly using
config for it.
Tricky moment about it is its return type which has to be
hagrid::initializers::Result. This return type is an alias to
anyhow::Result<Option<T>>. When construction fails for whatever reason it
returns Err. If everything went well, but construction is not a desired
behaviour, i.e. because of it was disabled in configuration, like we
do for "prometheus" initializer init() should return Ok(None).
Errors then handled in hagrid::initializers::run() function (by
initialize!() macro to be precise) and register the instance if it was
created.
register(rocket: Rocket<Build>, config: &Configuration, state_fairing_or_routes: <CONSTRUCTED_TYPE>)
Receives unwrapped instance of state, fairing or routes if it was
constructed and register it on rocket.
Some instances can be registered several times, e.g. like as fairing
and as routes. We do that for PrometheusMetrics.
- Move hagrid::web::ApplicationState (what was HagridState before
renaming) to hagrid::app::state::ApplicationState.
- Dissolve hagrid::web::rocket_factory() and set of configure_*
functions into initializers preserving the order of their registration
on rocket, though it might be not so important.
- Replace usage of hagrid::web::rocket_factory() in hagrid::web
module's tests with hagrid::app::configure_rocket() which become
approximate functional equivalent of it but calls for initializers to
populate the rocket.
Adhoc configuration value extraction from Figment is hard to reason on
application level, so it is better to create application wide
configuration. Apart of that having a unified module, like
hagrid::app::config, should allow to consolidate config logic in a
single place and simplify many other places.
Changes:
- Introduce hagrid::app::config module and Configuration struct in it.
- Migrate and consolidate adhoc config value extraction to a single
struct hagrid::app::config::Configuration.
- Replace logic to calculate default values in configuration in cases
when values were not provided with functionality from "serde" crate,
i.e. using #[serde(default)] or #[serde(default = ...)] to fill
missing values with defaults.
- Clean up signatures of configure_* like functions in hagrid::web::
module as some of them after moving configuration extraction can not
fail any more.
- Replace configuration propagation from pull to push model in
configure_* functions. Before the refactoring they all depend on
figment and configuration has been pulled from figment and put into
modules, now we explicitly push it. That's reduce dependency on global
configuration. That still might be reworked again once we move module
creation to hagrid::app module from hagrid::web.
This is to make "hagrid" code library alike instead of having it as a binary
which improves reusability and testability of the code.
So, the main.rs binary will be just a launcher for library code.
Changes:
- Extract modules declaration from main.rs to lib.rs in "hagrid" crate.
- Introduce "app" module which has to accumulate infrustructure code,
but for the time being it delegates run() and handle errors.
- Rename hagrid::web::serve() to hagrid::web::run() to standardize on
module entry function names.
- Accumulate rocket framework launching code in hagrid::web::run()
function.
To be able to move code around I have to replace usage of
macros like #[rocket::launch] or #[rocket::main] with direct usage of
:🚀:async_main, ignite() and launch() functions.
Previously, the test used Sequoia's high-level API which attached a
Direct-Key Signature to the key. As such, this wasn't a truly
naked-key.
This test uses low-level API to contruct a bare key and checks if the
import fails. Then it attaches a DKS which causes the import to
succeed. Then it revokes that key and checks if the revocation is
correctly propagated.
I believe KeyDatabase name is the thing of the past which was made to
simplify migration to Sqlite without doing the renaming all over code
base (which this commit does). So, this is a bit of technical debt IMO
and direct usage of Sqlite would serve better code readability.
KeyDatabase leaks presence of Sqlite though functions like new_file() and
others as such it doesn't serve very good the purpose of abstracting
things away and hiding the fact of Sqlite behind.
Code review issue: https://gitlab.com/keys.openpgp.org/hagrid/-/merge_requests/214#note_2484117659
Changes:
- Extract database file path and log dir path constructions from
Sqlite::new_file() and Sqlite::new_internal() associated functions to
Sqlite::db_file_path() and Sqlite::log_dir_path().
These path manipulations inside Sqlite/KeyDatabase wasn't ideal as
they increased diversity of reasons why Sqlite constructions can fail,
unnecessary constrain how Sqlite and with which paths can be used, and
that logic doesn't actually belong to Sqlite itself. So, later we
probably have to move these db_file_path() and log_dir_path()
functions else where.
- Replace base_dir parameter of Sqlite::new_file() with "db_file" and "log_dir"
params which gives us a way to use database file passed in command
line options.
- Add Sqlite::log_dir_path_from_db_file_path() associated function to
infer log_dir_path based on given db_file_path.
It is used in command handler where only db_file_path provided.
- Pull up log dir construction from Sqlite::new_internal() to
Sqlite::new_file() to avoid passing through log_dir_path through
several functions. This also makes Sqlite::new_file() signature
cleaner.
- Fix usages of Sqlite/KeyDatabase::new_file through out the code base:
and instead of providing base_dir, populate db_file_path and
log_dir_path using Sqlite::db_file_path() and Sqlite::log_dir_path()
functions which calculate paths based on provided base_dir.
- Remove "-c, --config" and "-e, --environment" options from
hagridctl::cli::Cli.
- Remove hagridctl::cli::DeploymentEnvironment enum.
- Remove entire hagridctl::config module as we don't extract paths from
configuration any more.
- Add "env" feature flag to "clap" dependency in hagridctl/Cargo.toml to
be able to populate Cli::db_file_path field from HAGRID_DB_FILE_PATH
environment variable.
- Add optional --db-file-path option to "hagridctl". Cli::db_file_path
is obligatory, but when --db-file-path is not provided Clap tries to
populate it from HAGRID_DB_FILE_PATH env var and if that's not
possible Clap terminates with error message.
- In hagridctl::cli::dispatch_cmd() use db_file_path from
cli.db_file_path instead of keys_internal_dir extracted from configuration.
- Replace keys_db_internal_dir with db_file_path in the following
command handlers:
- hagridctl::delete::run
- hagridctl::import::run
- Get rid of unnecessary Result unwrapping and wrapping it again in
hagrid::web::configure_db_service function and return the value from
KeyDatabase::new_file immediately instead.
Additional changes:
- In case of necessity specific binary can be provided for "just build"
recipe, e.g. "just build -p hagrid" or "just build -p hagridctl".
Code review issue: https://gitlab.com/keys.openpgp.org/hagrid/-/merge_requests/214#note_2482983036
Changes:
- Change type of "base_dir" argument of database::Sqlite::new_file()
associated function form "impl Into<PathBuf>" to "impl AsRef<Path>"
as AsRef<Path> is more generic.
- Remove "base" argument from "hagridctl delete" command.
- Extract "config" and "environment" options from "import" command to
hagridctl::cli::Cli struct.
- Make "config" and "environment" options global, i.e. they can be
specified with any command.
- Change "environment" from being argument to option as being argument
doesn't play well with "import" command where arbitrary list of
KEYRING_FILES can be specified. In general, being global works much
better with options then arguments.
So, <ENVIRONMENT> argument became "-e, --environment <ENVIRONMENT>" option
with the same default value (production).
- KEYRING_FILES in "import" command don't need to be "last" any more.
- Get "keys_db_internal_dir" in hagridctl::cli::dispatch_cmd() and let
it be propagated to places where KeyDatabase is created instead of
progapagation of config or base_dir.
Before merging "hagrid-delete" into "hagridctl" the later was
exclusively used with "import" command. So, it doesn't make a difference
how "config" and "ENVIRONMENT" parameters were specified. After merge
these arguments doesn't make sense at the top level any more, hence the
move.
Changes:
- Move "file" and "env" field from hagridctl::cli::Cli struct into
hagridctl::cli::Command::Import enum variant.
- Because of the move and optionality of "ENVIRONMENT" argument for
"import" command I had to add "last = true" option to KEYRING_FILES
argument which requires their specification to be prepended with --.
- Clean up hagridctl::cli::dispatch_cmd() function as match statement
now can take ownership over cli.command and "file" and "env" fields of
Command::Import can be destructured and passed directly to
config::load() instead of passing around reference to Cli struct.
Overall code becomes cleaner, IMO.
- Change config::load() to take "file" and "env" directly instead of
"cli" parameter.
- Get rid of run() function in hagridctl/src/main.rs as it becomes
redundant.
Code review issue: https://gitlab.com/keys.openpgp.org/hagrid/-/merge_requests/214#note_2482960066
Changes:
- Remove "hagrid-delete" bin declaration from Cargo.toml.
- Remove "clap" dependency for "hagrid" crate in Cargo.toml as after
moving "hagrid-delete" to "hagridctl" "hagrid" crate does not use
"clap" any more.
- Remove "run-hagrid-delete" recipe with its "hagrid-delete" and
"delete" aliases from justfile.
- Update Cargo.lock.
- Create "delete" command for "hagridctl" by adding cli::Command::Delete
variant which previously were hagrid-delete::cli::Cli struct.
- Move "delete" module with command handler ("run" function) from
"hagrid" crate to hagridctl::delete.
- Extend hagridctl::cli::dispatch_cmd() function with processing of
cli::Command::Delete variant and call to hagridctl::delete::run.
- Move print_errors() from hagrid::delete::cli module to hagrdictl::cli.
- Move KeyDatabase instantiation from hagrid/src/main.rs into
hagridctl::delete::run command handler as this way of database
instantiation is specific to "delete" command.
Probably later we have to reconsider how we instantiate database for
all the commands to avoid reimplementing that functionality every time
and duplicating the code.
That move caused change in signature of hagridctl::delete::run
function. Now we pass base path instead of db reference.
Commands:
just lint
Warnings:
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> hagridctl/src/cli.rs:39:58
|
39 | Command::Import { keyring_files } => import::run(&config, keyring_files.to_owned()),
| ^^^^^^^ help: change this to: `config`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `#[warn(clippy::needless_borrow)]` on by default
I think it looks a bit weird to have things like module "import" with function
"do_import" which is a command handler. So, I renamed all such command
handlers to simple "run" functions. IMO on call side it looks now
better, e.g.: import::run().
Changes:
- Rename the following command handler functions:
- hagridctl::import::do_import -> hagridctl::import::run
- hagrid::delete::delete:delete -> hagrid::delete::delete::run
(still weird, but minus one delete. First delete in path is bin
name, second - module for command handler)
- tester::generate::do_generate -> tester::generate::run
- tester::genreqs::do_genreqs -> tester::genreqs::run
Changes:
- Add "derive" and "unicode" features for "clap" in Cargo.toml.
- Remove dependency on "structopt" crate from Cargo.toml.
- Update Cargo.lock.
- Rename crate::delete::cli::Opt struct to Cli.
- Replace usage of #[structopt()] macros with #[derive(Parser)].
- Rename variables and function parameters from "opt" to "cli"
accordingly.
- Don't parse opt.query in dispatch_cmd function. As Opt.query field
type was replaced from String to Query in Cli.query new version of
Clap automatically determines value_parser and calls FromStr::from_str
implementation to convert str slice into field of type Query.
- Replace improrts of structopt::StructOpt with clap::Parser.
Changes:
- Add "derive" feature for "clap" in hagridctl/Cargo.toml.
- Move about description from hagridctl/src/cli.rs to "description" field in
hagridctl/Cargo.toml. So, "clap" can access it via #[command(about)]
macro configuration.
- Update Cargo.lock.
- Replace in hagridctl/src/cli.rs usage of clap::App from Clap 2.34.0
with clap::Parser derive macro based API, cli::Cli struct and cli::Command
enum.
- Omit explicit specification of args name which resulted in upper
casing cmd option's parameters, e.g. "<keyring files>" -> "<KEYRING_FILES>".
- Replace in cli::dispatch_cmd() "if let" based logic with simple match
on cli::Command enum variants.
- Get rid of manual parsing of options' and commands' values in
cli::dispatch_cmd(). As it is hadled by derived
value_parser (https://docs.rs/clap/latest/clap/_derive/index.html#arg-attributes)
based on field types.
- Replace set of limited values specified with .possible_values() method
call with #[arg(value_enum)] macro and hagridctl::cli::DeploymentEnvironment
enum. I make environment names fully qualified, but left abbreviations
as aliases, so `just hagridctl dev import` kind of commands still
allowed.
- Replace in hagridctl/src/main.rs cli::app().get_matches() (which is Clap
v2 API) with cli::Cli::parse() (Clap v4 API).
- Add "unicode" feature flag for clap dependency in hagridctl/Cargo.toml.
- Refactor config::load() and to use cli::Cli struct instead of
clap::ArgMatches. Defaulting to Rocket.toml for configuration file was
moved to default value for hagridctl::cli::Cli.env field.
- Replace .unwrap() calls in config::load() function with ? operator.
- Extract match statement from config::load() to
HagridConfigs::get_by(deployment_environment) method.
