Changes:
- Split hagrid::web::tests::maintenance() tests to the following:
- hagrid::routes::vks:tests::get_upload:::maintenance()
- hagrid::routes::manage::tests::get_manage::maintenance()
- hagrid::routes::vks:tests::get_verify_token:::maintenance()
- hagrid::routes::pks::tests::post_pks_add_multipart_form_data::maintenance()
- hagrid::routes::api::rest::vks::tests::post_vks_v1_upload::maintenance()
- hagrid::routes::api::rest::vks::tests::post_vks_v1_request_verify_json::maintenance()
- hagrid::routes::vks::tests::put_root::maintenance()
- Fix tests' endpoints to make them more accurate and actually match
existing endpoints:
- GET /verify -> GET /verify/<token>
- GET /pks/add -> POST /pks/add/ [Content-Type: multipart/form-data]
- GET /vks/v1/upload -> POST /vks/v1/upload
- GET /vks/v1/request-verify -> POST /vks/v1/request-verify
- Verify that maintenance state has dissapeared after maintenance file
removal in each individual test with the same endpoint on which we
asserted maintenance mesage.
Previous tests checked only maintenance message disappearance on GET /upload,
which wasn't completely correct.
- Introduce new fixtures at hagrid::routes::tests::common:
- token
- serialized_cert
- key_multipart_form_data
- Introduce new test module: hagrid::routes::tests::common::test.
The module is going to accumulate common tests which define common
workflow for the test and can be called from other tests to check for
specific behaviour of concreate function or HTTP endpoint.
- Generalize functionality of maintenance test. Move it to module with
shared tests: hagrid::routes::tests::common::test::maintenance().
Changes:
- Extract and generalize assert for absence of the maintenance text from
hagrid::web::tests::maintenance() test to
hagrid::routes::tests::common::assert::response_does_not_contain_text()
assert helper function. Reuse it in the test instead.
- Refactor hagrid::web::tests::common::assert::check_maintenance()
function to make it more reusable and remove code duplication in its
implementation:
- Instead of duplicating response asserting code call for the helper
hagrid::routes::tests::common::assert::response() and prefill the
status.
- Make it accept response instead of passing in client and uri. This
make check_maintenance() helper more reusable as now we are not
dependent on what HTTP verb is used to get the response. In
addition, request occurs directly inside body of the test and helper
just assert the response. Hence, reparation of responsibilities and
following SRP.
- Let check_maintenance() accept the maintenance text which is checked
in response. The previous implementation of the
hagrid::web::tests::maintenance() test and the helper was frigile as
it allows a lot of possibilities for maintenance text to go out of
coherence.
- Adjust code in hagrid::web::tests::maintenance() test according to
changed signature of check_maintenance() assertion helper.
- Remove hardcoded checks in hagrid::web::tests::maintenance() test and
reuse assertion helpers.
Changes:
- Move tests of hagrid::web::tests::check_response() for /search
endpoint to hagrid::routes::vks::tests::get_search::not_supported_search_query().
Refactor tests along the way to get rid of boilerplate code.
- Move test of hagrid::web::tests::check_response() for /pks/lookup
endpoint to hagrid::routes::pks::tests::get_pks_lookup::not_supported_search_query().
Refactor tests along the way to get rid of boilerplate code.
- Move test of hagrid::web::tests::check_response() for
/.well-known/openpgpkey/<domain>/<policy> endpoint to
hagrid::routes::wkd::tests::get_wkd_policy::wkd_policy_respond_successfully_with_empty_body().
Refactor tests along the way to get rid of boilerplate code.
Changes:
- Replace usage of hagrid::web::tests::common::assert::check_response()
with hagrid::routes::tests::common::assert::response() to remove code
duplication in tests.
- Refactor hagrid::web::tests::check_response() tests to use
assert::response() helper.
- Take &str instead of &'static str as present_page_text argument in
assert::response() helper. This was a bug. It affected reusability of
the assertion helper function.
"rocket" fixture ATM used only by "client" fixture. So, it can be
internal to the hagrid::routes::tests::common module. Its export caused
clash of names with "rocket" module from "rocket" crate which forced to
prefix module from crate with ::, i.e. ::rocket.
Using absolute path to the module is probably a good thing by itself
when it used consistently across code base, but in Hagrid's code base it
wasn't and being forced to do that by that tricky coincidence was
annoying and confusing.
There several caveats to the refactoring:
- Before refactoring all tests ran in strong order as they were
sequential code in a single function. I believe this was an accident
behaviour and there were nothing in functionality of basics() fn that
forced certain sequence of test execution. So, using table testing
with random order considered by me fine.
- basics() fn tests only read queries. So, it was a discovery for me to
find at the end of the function check for consistency,
(i.e. assert_consistency()) as it logically does not make sense.
I believe that was a test code copy-paste mistake.
To preserve basic check I moved it to dedicated test basic_consistency().
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.
