Testing the application

[robinkrahl]

I think we should try to test the command-line interface using integration tests. We could either manually execute the command or use the assert_cmd crate.

The most complex part is dealing with the different scenarios 1) no device present, 2) Nitrokey Pro present, 3) Nitrokey Storage present, and potentially 4) several devices present. For nitrokey, I defined features for the device types and prefixed every test with a conditional ignore, for example:

#[test]
#[cfg_attr(not(any(feature = "test-pro", feature = "test-storage")), ignore)]
fn hotp_no_pin() {
    // ...
}

I’m not really happy with this solution, but it was the best option I could think of. What are your thoughts on this issue?

[d-e-s-o]

I think we should try to test the command-line interface using integration tests. We could either manually execute the command or use the assert_cmd crate.

Thanks for bringing up this issue.

I agree the current set of tests we have is not great and the overall approach to testing leaves a lot to be desired. Indeed, this crate looks promising and would help us test in an end-to-end fashion. I see that it has a lot of (transitive) dependencies (including serde), but I guess that's okay as it's only for testing.
I am currently waiting for response from the nitrokey team on their offer of providing a stick for testing. Right now I haven't run the tests at all in a while, because I only have a "production" stick and I don't want to take risks with it.

The most complex part is dealing with the different scenarios 1) no device present, 2) Nitrokey Pro present, 3) Nitrokey Storage present, and potentially 4) several devices present. For nitrokey, I defined features for the device types and prefixed every test with a conditional ignore, for example:

#[test]
#[cfg_attr(not(any(feature = "test-pro", feature = "test-storage")), ignore)]
fn hotp_no_pin() {
    // ...
}

I’m not really happy with this solution, but it was the best option I could think of. What are your thoughts on this issue?

I also don't see too many options. I'd prefer a runtime based approach, though. Meaning we would detect the type(s) of stick(s) present and skip tests that are not supported. The nitrocli application is supposed to work with both so I don't see a good reason for compiling tests for it conditionally.

That raises an interesting question on how we should treat cases where both a Pro and a Storage stick are present.
From a brief glance it appears that from the nitrokey crate side it should not matter, but we would have to decide on which one to use if really two are present and a test is eligible to run on both. But I guess for the sake of coverage we should run on both anyway.

Perhaps we can come up with a solution such as the following:

#[test]
// Actually run the test on both devices, if present. That is, run it twice. If only one is present run on that one. If none, skip.
#[accept(Pro, Storage)]
fn hotp_no_pin() {
    // ...
}

#[test]
// Only run on a storage device, if present. Skip the test otherwise.
#[accept(Storage)]
fn open_encryped_volume() {
    // ...
}

This is more pseudo code and idealistic thinking than anything else. I am not sure how feasible an attribute based implementation is.

[robinkrahl]

I am currently waiting for response from the nitrokey team on their offer of providing a stick for testing. Right now I haven't run the tests at all in a while, because I only have a "production" stick and I don't want to take risks with it.

Which model did you request?

I also don't see too many options. I'd prefer a runtime based approach, though. Meaning we would detect the type(s) of stick(s) present and skip tests that are not supported. The nitrocli application is supposed to work with both so I don't see a good reason for compiling tests for it conditionally.

Just to be clear: The ignore approach is not conditional compilation but conditional execution (that has to be specified at compile time). Currently, we could also set the Pro as the baseline and enable Storage-specific code using a feature, but I want to be prepared in case there are differences between the Pro and the Storage. (As far as I know, there are already minor differences, e. g. the Pro supports 64-bit timestamps for TOTP, but the Storage does not.)

That raises an interesting question on how we should treat cases where both a Pro and a Storage stick are present.
From a brief glance it appears that from the nitrokey crate side it should not matter, but we would have to decide on which one to use if really two are present and a test is eligible to run on both. But I guess for the sake of coverage we should run on both anyway.

From the libnitrokey side, NK_login_auto prefers the Pro over the Storage. NK_login and NK_login_enum let the user choose the model. This corresponds to nitrokey::connect() and nitrokey::{Pro, Storage}::connect(). In the nitrokey tests, I try to use the model-specific connect most of the time.

In general, I think that there is still a lot of work to do on libnitrokey to properly support multiple sticks connected to the machine. There is a mechanism for the Storage (based on the CPU id), but not for the Pro.

Perhaps we can come up with a solution such as the following:

#[test]
// Actually run the test on both devices, if present. That is, run it twice. If only one is present run on that one. If none, skip.
#[accept(Pro, Storage)]
fn hotp_no_pin() {
    // ...
}

#[test]
// Only run on a storage device, if present. Skip the test otherwise.
#[accept(Storage)]
fn open_encryped_volume() {
    // ...
}

This is more pseudo code and idealistic thinking than anything else. I am not sure how feasible an attribute based implementation is.

Yes, that would be nice.

[d-e-s-o]

Thanks for the clarification, Robin.

I am currently waiting for response from the nitrokey team on their offer of providing a stick for testing. Right now I haven't run the tests at all in a while, because I only have a "production" stick and I don't want to take risks with it.

Which model did you request?

I asked for a Storage stick just because right now that is what the application uses and is designed for. But I mentioned that having a Pro device in addition to that would help in the near-term future.

[robinkrahl]

I’m using my Pro (v1) “in production” and my Storage (v2) for development and testing. So it would make sense for one of us to get a Pro for development some day. I can do some tests with my Pro, but I will not run the full test suite often.

[d-e-s-o]

There may be another issue with respect to tests and that is that cargo test runs them concurrently. The stick could not handle that. Back in the day I worked around that by using RUST_TEST_THREADS=1 because it would be only the tests that potentially issue commands in parallel. I did not like that work around, though (mostly because I created a Makefile for that and typing cargo test is the more ingrained way for running tests)

I think that even if the nitrokey crate does provide synchronization (does it?) that will only ensure that individual operations do not clash, but higher level sequences (e.g., two threads opening and closing the encrypted volume) would still be a recipe for disaster. What we would need is the concept of a transaction or something along those lines. But I guess for the tests we could get away with a global lock that is held on a per-test basis.

[robinkrahl]

Currently, nitrokey does not provide synchronization. I intend to add it, but I still have to do some research on the implementation. For the nitrokey tests, I use the --test-threads 1 option.

[d-e-s-o]

Perhaps we can come up with a solution such as the following:

#[test]
// Actually run the test on both devices, if present. That is, run it twice. If only one is present run on that one. If none, skip.
#[accept(Pro, Storage)]
fn hotp_no_pin() {
    // ...
}

#[test]
// Only run on a storage device, if present. Skip the test otherwise.
#[accept(Storage)]
fn open_encryped_volume() {
    // ...
}

This is more pseudo code and idealistic thinking than anything else. I am not sure how feasible an attribute based implementation is.

Yes, that would be nice.

I think we can come pretty close to that (or make it even better). I just experimented with a procedural macro for a custom attribute. I have a crude prototype (not even worth showing :D) but I think we should be able to get to something like:

  #[test_device]
  fn storage_status(device: nitrokey::Storage) {
    assert!(false, "I run (and fail) on a Storage device, and only there")
  }

  #[test_device]
  fn pro_status(device: nitrokey::Pro) {
    assert!(false, "I run (and fail) on a Pro device, and only there")
  }

  #[test_device]
  fn any_status(device: nitrokey::DeviceWrapper) {
    assert!(false, "I run (and fail) on any Nitrokey device")
  }

The main thing I wanted to verify here was that we can essentially emit a custom #[test] decorated function and it will be expanded properly (not shown here). I believe the rest is just a matter of doing it (a bit of massaging of the token stream we get to emit one or two functions, querying the available devices & passing them to it; I have some experience with procedural macros, so I am not too worried about that).

I'll also see if we can fudge the serialization issue with such a mechanism (essentially by just having a global lock that is grabbed for each function; I'd much rather have that as opposed to pushing the task of setting the thread count to the user, with god knows what happening if he/she forgets).

[robinkrahl]

Looks great! How do you choose which tests to execute?

[d-e-s-o]

Basically all that happens is that we generate one or two wrappers around the provided test function. These are just ordinary Rust test functions, i.e., they will just appear and feel like a regular test function. That also means that you can pattern match on them when invoking them through cargo test.

Here is a more polished version: 9aa02e2

$ cargo test
running 11 tests
...
test commands::tests::any_device_serial_pro ... FAILED
test commands::tests::storage_serial_verbose ... FAILED
test commands::tests::any_device_serial_storage ... ok

(ignore the test failures, they are due to missing synchronization)

It works, but it does not yet account for a device not being present and skipping the test. There are probably a few more rough edges.

My question to you is whether functionality like this is something you would want for the nitrokey crate as well. If I go with this approach for testing I will have to publish the crate anyway, so I would make the name (nitrocli-test or nitrokey-test) dependent on whether you plan to use it.

[robinkrahl]

My question to you is whether functionality like this is something you would want for the nitrokey crate as well. If I go with this approach for testing I will have to publish the crate anyway, so I would make the name (nitrocli-test or nitrokey-test) dependent on whether you plan to use it.

Yes, it would be great if nitrokey could use the mechanism! Yet the nitrokey probably has some additional requirements. I’ll think about it and add a comment with more details.

[d-e-s-o]

Awesome! I verified that serialization of all tests is possible and have it implemented already. I will implement a bunch of tests on top of it, both on the nitrokey and nitrocli side and see how things look and what may be missing.

[d-e-s-o]

I got a version of the nitrokey device tests converted to using the new nitrokey-test crate. With it, conceptually all you need to run the tests is cargo test. No more features, no more worrying about threads. Please have a look at e7f4b66 if you have time and let me know if you have suggestions @robinkrahl

(the full patch set can be found on the topic/nitrokey-test branch)

A few unordered thoughts from my side:

• I think if you decide to upgrade nitrokey to Rust 2018 we can export the nitrokey-test::test_device function as nitrokey::test (serde does something similar)
• nitrokey should probably provide a proper error code when trying to connect to a device that is not present; Unknown seems too unspecific, in my opinion
• I am not sure how best to indicate that a test has been skipped (due to a device not being present); Rust does not seem to provide a supported way :-/

[robinkrahl]

I got a version of the `nitrokey` device tests converted to using the new `nitrokey-test` crate. With it, conceptually all you need to run the tests is `cargo test`. No more features, no more worrying about threads. Please have a look at e7f4b66 if you have time and let me know if you have suggestions @robinkrahl (the full patch set can be found on the [topic/nitrokey-test](https://github.com/d-e-s-o/nitrocli/commits/topic/nitrokey-test) branch)

That sounds great! I’ll have a look at it once I finished the ongoing implementation and documentation issues.

- I think if you decide to upgrade `nitrokey` to Rust 2018 we can export the `nitrokey-test::test_device` function as `nitrokey::test` (serde does something [similar](https://github.com/serde-rs/serde/blob/47e238aa13523fc98bfa540a9f0398c73ad52cf4/serde/src/lib.rs#L292))

The code on the next branch is already upgraded to Rust 2018.

- `nitrokey` should probably provide a proper error code when trying to connect to a device that is not present; `Unknown` seems too unspecific, in my opinion

Yes, that’s one point on my to-do list. I was considering returning an Option<...> instead. What do you think about that? We do not receive any error code from libnitrokey, so that might be more appropriate. I am planning to release version 0.2.2 soon. Besides implementing `Copy` for some types, do you have anything I should include? (I won’t have time to look at `nitrokey-test` before that, so that will come with the next release.)

[d-e-s-o]

The code on the next branch is already upgraded to Rust 2018.

I'll check that out. Thanks for the pointer!

Yes, that’s one point on my to-do list. I was considering returning an Option<...> instead. What do you think about that? We do not receive any error code from libnitrokey, so that might be more appropriate. I am planning to release version 0.2.2 soon.

Yeah, I think that makes sense. But, wouldn't that have to be considered a compatibility breaking change? Do you really plan to go with 0.2.2 in that case?

Besides implementing Copy for some types, do you have anything I should include? (I won’t have time to look at nitrokey-test before that, so that will come with the next release.)

I can't think of anything else right now, but I also haven't worked closely with the crate to be honest.

[robinkrahl]

Yeah, I think that makes sense. But, wouldn't that have to be considered a compatibility breaking change? Do you really plan to go with `0.2.2` in that case?

I’ll release 0.2.2. with the minor changes already published on the next branch. 0.3.0 will follow soon, but there are some other breaking changes I want to include: I’ll change `set_time` to `set_time(force: bool)`. Also, I am considering not to re-export the public interface in the main module and to rename the getter methods according to the style guide.

[robinkrahl]

Could you add tests that are executed if no device is connected (maybe if the function does not have any arguments)? That would be important for the nitrokey crate.

Also, would it be possible to force nitrokey-test to run one of the test suites? While it is handy for a user to just execute cargo test, I’d like to make sure that the correct test suite is run without manually checking the output.

[d-e-s-o]

Could you add tests that are executed if no device is connected (maybe if the function does not have any arguments)? That would be important for the nitrokey crate.

Isn't that exactly what the existing #[test] does for you? In what way is it insufficient?

Also, would it be possible to force nitrokey-test to run one of the test suites? While it is handy for a user to just execute cargo test, I’d like to make sure that the correct test suite is run without manually checking the output.

What test suites exactly are you referring to? Storage vs. Pro vs. No-device? Why have that concept to begin with if it is not needed?

[robinkrahl]

Isn't that exactly what the existing #[test] does for you? In what way is it insufficient?

#[test] is always executed. But I have to test that nitrokey does not connect if there is no device.

What test suites exactly are you referring to? Storage vs. Pro vs. No-device? Why have that concept to begin with if it is not needed?

Yes. The point of having tests is that something might go wrong, even when connecting. Currently I can verify that the appropriate tests have been executed by checking the output, but I can’t express that I expect the Pro tests to be executed. Also it would improve the execution time by removing one command per test.

Don’t get me wrong, these are no big issues for me. But it would be handy if there was an easy solution.

[d-e-s-o]

Isn't that exactly what the existing #[test] does for you? In what way is it insufficient?

#[test] is always executed. But I have to test that nitrokey does not connect if there is no device.

So you want the test to run if the connection failed to begin with, because that's when no device is present. Yeah, that can be done.

What test suites exactly are you referring to? Storage vs. Pro vs. No-device? Why have that concept to begin with if it is not needed?

Yes. The point of having tests is that something might go wrong, even when connecting. Currently I can verify that the appropriate tests have been executed by checking the output, but I can’t express that I expect the Pro tests to be executed. Also it would improve the execution time by removing one command per test.

I don't think we can have both, tests that detect availability of a device at runtime and ones that will fail if a device is not present. Well, we could better if there was an API to check for existence of a device of a particular model. But for lack of that we have to assume that a connection succeeds if and only if a device is present (though we could certainly request such a function).

I prefer having tests that run without user configuration and test what ever can be tested, but I can see it being a trade off.

[d-e-s-o]

I guess another possibility (and perhaps that's closer to what you were getting at) is to layer additional feature recognition on top of the detection logic. So what I have would be the default behavior, but if you run cargo test --feature=pro (or whatever) it would skip the Nitrokey Storage tests without checking.

I think that should not be hard.

[robinkrahl]

So you want the test to run if the connection failed to begin with, because that's when no device is present. Yeah, that can be done.

Yeah, exactly. The current tests won’t be very meaningful – assert that the connection fails (unit test) if the connection failed (execution guard). But I try to have complete tests, and there might be more interesting test cases.

So what I have would be the default behavior, but if you run cargo test --feature=pro (or whatever) it would skip the Nitrokey Storage tests without checking.

Yes, that’s what I was thinking about. I would prefer to have a run-time option (or an environment variable), but features would also be an option.

[d-e-s-o]

Okay, I first want to release a new version of nitrocli and then I can focus on nitrokey-test. I do realize that ultimately it is much more suited to running the nitrokey tests than it is for testing nitrocli itself. I haven't really been able to come up with a good testing strategy for nitrocli to be honest.

[d-e-s-o]

I am planning to release version 0.2.2 soon. Besides implementing Copy for some types, do you have anything I should include? (I won’t have time to look at nitrokey-test before that, so that will come with the next release.)

I am not sure it is a good idea to convert errors like you do here: https://github.com/d-e-s-o/nitrocli/blob/master/nitrokey/src/util.rs#L114

Wouldn't it be better to pass through the original error instead? Arguably, that's not the most significant problem here because the error does not contain too much additional information, but in general I don't think you should throw away the original.

[d-e-s-o]

Also, here you probably want to preserve the original error code. Now I have no way of knowing what exactly went wrong when I get an Unknown :-(

[robinkrahl]

I am not sure it is a good idea to convert errors like you do here: https://github.com/d-e-s-o/nitrocli/blob/master/nitrokey/src/util.rs#L114

Wouldn't it be better to pass through the original error instead? Arguably, that's not the most significant problem here because the error does not contain too much additional information, but in general I don't think you should throw away the original.

As far as I remember, this is the only instance where we convert an internal error into a CommandError. (RngError is obselete in newer versions.) If we would keep the error, we would have two errors types for the same error, depending on whether we catch it or whether libnitrokey catches it. So I’d prefer to keep it as it is.

Also, here you probably want to preserve the original error code. Now I have no way of knowing what exactly went wrong when I get an Unknown :-(

Good point, I’ll change that. For the time being, you can use nitrokey::set_debug to see the communication (and the error code).

[d-e-s-o]

Good point, I’ll change that.

I actually already have a change that addresses that shortcoming, so don't worry about it. What's your preferred way to receive contributions? Just e-mail?

[robinkrahl]

I prefer patches via mail (git format-patch). If that’s too cumbersome, git request-pull via mail is fine too. In case you already have a patch for the InvalidString error too, feel free to include it and I’ll have a look.

[robinkrahl]

I haven't really been able to come up with a good testing strategy for nitrocli to be honest.

Here is an example how I’d write the tests: tests/otp.rs. I think it is not necessary to test every character of the status output (unless we want to guarantee a specific format), but we should make sure that the basic operations work as intended.

[d-e-s-o]

I haven't really been able to come up with a good testing strategy for nitrocli to be honest.

Here is an example how I’d write the tests: tests/otp.rs. I think it is not necessary to test every character of the status output (unless we want to guarantee a specific format), but we should make sure that the basic operations work as intended.

Thanks for looking into this topic! The problem I have is less with command execution and output retrieval. The problem is with how to determine which device to use etc. (basically what we discussed somewhere above).
Ideally I would want to use nitrokey-test which does the device detection for me and ensures that tests are serialized. But that only works if we can inject the device to use, which we can't if we just invoke the command.
This may be possible to solve after some refactoring of the code (and then invoking strategically designed entry points that are close enough to what happens when the program is started as a process), but I haven't had time to look at it.

[robinkrahl]

Maybe I am missing something, but I don’t see the problem with using nitrokey-test. I think we should just assume that there is only one device connected to the system (AFAIR we already discussed that somewhere). Then we could use nitrokey-test without modifications, or maybe in a version that does not require the test function to take an argument.

The test mentioned above would be:

#[test_device]
fn totp_no_pin(_: nitrokey::DeviceWrapper) {
}

or better:

#[test_device(DeviceWrapper)]
fn totp_no_pin() {
}

[d-e-s-o]

I think we should just assume that there is only one device connected to the system (AFAIR we already discussed that somewhere).

I am not aware of that having been discussed. Yeah, that would work, but my goal was to support two devices being present.

[robinkrahl]

I am not aware of that having been discussed. Yeah, that would work, but my goal was to support two devices being present.

Do you want to support it for nitrocli in general or for the tests? I don’t think it makes sense to implement this functionality only for the tests – and if we would support it for nitrocli in general, we could pass the appropriate flags in the tests.

Regarding the previous discussions: We were talking about the support of multiple devices earlier in this thread. I originally wanted to comment some more on that issue, but apparently I deleted it as I considered it to be off-topic. We have three strategies:

1. Assume only one connected device.
2. Support both a Pro and a Storage at the same time.
3. Support multiple arbitrary devices at the same time.

My preferred solution is the third, but we don’t have the library support to implement it. We can implement the second, but that requires some refactoring. I’d add top-level --storage and --pro options to select the device. The only problem is storing the top-level options. (I’d rather avoid passing them all the way from args::handle_arguments to commands::*.)

[d-e-s-o]

I think we should just assume that there is only one device connected to the system (AFAIR we already discussed that somewhere).

I am not aware of that having been discussed. Yeah, that would work, but my goal was to support two devices being present.

I'll respond to your questions in a bit in more detail, but let me first clarify that this statement was only meant for the testing situation (I implicitly assumed that because this issue is about testing, but we are now also touching on nitrocli itself). I want tests to run on both devices, if they are present, to have the full test coverage without replugging anything. It was not meant as a statement as to whether nitrocli proper should support two devices at the same time.

[d-e-s-o]

I am not aware of that having been discussed. Yeah, that would work, but my goal was to support two devices being present.

Do you want to support it for nitrocli in general or for the tests? I don’t think it makes sense to implement this functionality only for the tests – and if we would support it for nitrocli in general, we could pass the appropriate flags in the tests.

Basically I haven't really thought about nitrocli. I am about to finish my changes that move the nitrokey-rs tests to nitrokey-test. I am really not happy with what the Rust test framework allows us to do, but it works (the end result and the biggest caveat being no clean indication as to whether a test was skipped or not).

Regarding the previous discussions: We were talking about the support of multiple devices earlier in this thread. I originally wanted to comment some more on that issue, but apparently I deleted it as I considered it to be off-topic. We have three strategies:

1. Assume only one connected device.
2. Support both a Pro and a Storage at the same time.
3. Support multiple arbitrary devices at the same time.

My preferred solution is the third, but we don’t have the library support to implement it. We can implement the second, but that requires some refactoring. I’d add top-level --storage and --pro options to select the device. The only problem is storing the top-level options. (I’d rather avoid passing them all the way from args::handle_arguments to commands::*.)

I am honestly not sure if supporting multiple devices from nitrocli is necessary (options two and three). I consider it very unlikely that a user has both a Storage and a Pro stick or even multiple sticks of one type.

For nitrocli itself I would honestly just stick to the first option for the sake of simplicity. If we get that done well that will probably cover 98% of users and the remaining can still just change sticks physically (which is a commonly enough operation to not introduce too much additional pain -- at least based on my experience).

Granted, if --pro and --storage options are reasonably simple to implement and don't affect code complexity negatively too much, there is not much harm in adding them, but really it seems like a rarely used feature. I do see value in having them for testing, though, (and I guess that's where you may be coming from as well).
So from a testing angle there is some merrit, but we could equally well (again, haven't dug deep here) just have an internal structuring that lends itself to passing in the desired device; this does not have to be user-facing [the trade-offs would presumably be largely the same, though]).

[robinkrahl]

I think the code structure would be similar for both approaches: Either we have some kind of mutable global variable indicating the device type, or an additional argument for the command handlers in the commands crate. (We might even need this kind of code if we introduce other top-level options, for example --debug or --verbose, that affect the commands.) Therefore I’d prefer to implement it as a nitrocli feature – there might be someone who can benefit from it, and it would IMHO be the cleaner solution for the tests.

[d-e-s-o]

Yeah, sounds good. I think the context logic I added with my last pull request will make this differentiation trivial. Do you already have --pro and --storage implemented? What about tests? You've provided the otp example, are you planning on creating a pull request? I'll also look into testing next, so let me know to not duplicate efforts.

[robinkrahl]

I don’t have anything prepared. The OTP example was just intended as a starting point for further discussion. Just let me know what you are working on and what I could do.

[d-e-s-o]

If you want you can add the --pro and --storage options. My plan is to merge the -v/--verbose logic, then adjust nitrokey-test to support attribute arguments (the #[nitrokey_test::test(DeviceWrapper)] syntax you suggested), and then get a first test implemented (probably in that order).
The next thing (unrelated to tests) I'd like to get hooked up is support for changing the update PIN (I'd probably go with firmware PIN) -- it should fit reasonably well into what we already have.

Perhaps it's best to just make a list of open items and before anyone starts working on any we just compare(-the-assignee)-and-swap(-our-name) in there. What do you think? Shall we repurpose issue #40 for that?

[robinkrahl]

If you want you can add the --pro and --storage options.

Okay, I’ll do that. But I think I’ll use --model [pro|storage] instead.

My plan is to merge the -v/--verbose logic, then adjust nitrokey-test to support attribute arguments (the #[nitrokey_test::test(DeviceWrapper)] syntax you suggested), and then get a first test implemented (probably in that order).

I noticed that we still have to know for which device we execute the test. Otherwise we can’t pass the appropriate flags to nitrocli. So it might be better to have (similar to the DeviceWrapper case):

#[nitrokey_test::test]
fn test(model: nitrokey::DeviceModel) {
    // execute `nitrocli --model <model>`
}

The next thing (unrelated to tests) I'd like to get hooked up is support for changing the update PIN (I'd probably go with firmware PIN) -- it should fit reasonably well into what we already have.

I’m still not sure whether we should call it PIN or password. PIN would be consistent with the user and admin PIN, but it is not managed by the smart card and not blocked after three retries (which for me is the main difference between a PIN and a password).

Perhaps it's best to just make a list of open items and before anyone starts working on any we just compare(-the-assignee)-and-swap(-our-name) in there. What do you think? Shall we repurpose issue #40 for that?

Good idea, but I cannot edit your issue. Can you edit my comments? Otherwise we could use a Github wiki page (if they still exist).

[d-e-s-o]

If you want you can add the --pro and --storage options.

Okay, I’ll do that. But I think I’ll use --model [pro|storage] instead.

👍

I noticed that we still have to know for which device we execute the test. Otherwise we can’t pass the appropriate flags to nitrocli. So it might be better to have (similar to the DeviceWrapper case):

#[nitrokey_test::test]
fn test(model: nitrokey::DeviceModel) {
    // execute `nitrocli --model <model>`
}

Okay. I was thinking along the lines of:

fn test_impl(arg: String) {
   ...
}
#[nitrokey_test::test(nitrokey::Pro)]
fn pro_test() {
  test_impl("--model=pro")
}
#[nitrokey_test::test(nitrokey::Storage)]
fn storage_test() {
  test_impl("--model=storage")
}

but given that nitrokey already has a DeviceModel using it will be less verbose.

The next thing (unrelated to tests) I'd like to get hooked up is support for changing the update PIN (I'd probably go with firmware PIN) -- it should fit reasonably well into what we already have.

I’m still not sure whether we should call it PIN or password. PIN would be consistent with the user and admin PIN, but it is not managed by the smart card and not blocked after three retries (which for me is the main difference between a PIN and a password).

Yeah I thought about that, too. In my opinion the usage of user PIN/admin PIN is already a misnomer. From what I understand the entered string does not have to be a number (as in Personal Identification Number). I am not sure whether PIN evokes associations with the smart card per se.

The other question (and perhaps I would make the answer to the above dependent on its answer) is whether to put this functionality below nitrocli pin or nitrocli storage. I haven't thought about that enough to have a firm opinion, but I see arguments for either.

Perhaps it's best to just make a list of open items and before anyone starts working on any we just compare(-the-assignee)-and-swap(-our-name) in there. What do you think? Shall we repurpose issue #40 for that?

Good idea, but I cannot edit your issue. Can you edit my comments? Otherwise we could use a Github wiki page (if they still exist).

Hm. Yeah, I can edit your comments. Okay, a wiki page is fine (should still be available)! Let me quickly create something.

[d-e-s-o]

I've created https://github.com/d-e-s-o/nitrocli/wiki Let me know if there are problems with it. Improve at will.

[d-e-s-o]

I noticed that we still have to know for which device we execute the test. Otherwise we can’t pass the appropriate flags to nitrocli. So it might be better to have (similar to the DeviceWrapper case):

#[nitrokey_test::test]
fn test(model: nitrokey::DeviceModel) {
    // execute `nitrocli --model <model>`
}

I looked at this proposal a bit more and to me it seems not like a really great model to work with. Basically, while the proposed nitrokey::DeviceModel signature captures the case where a test can run on any device, how would we represent the case where it is eligible only for running on a Pro device?

That:

#[nitrokey_test::test]
fn test(model: nitrokey::DeviceModel::Pro) {
    // ...
}

Isn't really valid Rust and while it does not matter much because the attribute macro can massage it to the right thing, it seems weird to me.

The alternative is

#[nitrokey_test::test(Pro)]
fn test() {
    // ...
}

but that doesn't really pair nicely with the aforementioned signature.

However, I believe we can just use the nitrokey-test crate as-is without requiring much syntactic sugar:

  trait IntoArg {
    fn into_arg(self) -> &'static str;
  }

  impl IntoArg for nitrokey::Pro {
    fn into_arg(self) -> &'static str {
      "--model=pro"
    }
  }

  impl IntoArg for nitrokey::Storage {
    fn into_arg(self) -> &'static str {
      "--model=storage"
    }
  }

  impl IntoArg for nitrokey::DeviceWrapper {
    fn into_arg(self) -> &'static str {
      match self {
        nitrokey::DeviceWrapper::Pro(x) => x.into_arg(),
        nitrokey::DeviceWrapper::Storage(x) => x.into_arg(),
      }
    }
  }

  #[test_device]
  fn foobar(device: nitrokey::DeviceWrapper) {
    let args = ["nitrocli", device.into_arg(), "status"];
    assert!(run(&args[..]).is_ok());
  }

  #[test_device]
  fn foobarbaz(device: nitrokey::Storage) {
    let args = ["nitrocli", device.into_arg(), "storage", "status"];
    assert!(run(&args[..]).is_ok());
  }

IntoArg will consume the device (and, hence, disconnect) while converting it into something we can work with. Given that we have to connect anyway (as there is no function that checks for availability of a device without connecting), there shouldn't be any avoidable overhead in there.

[d-e-s-o]

I shamelessly copied from your otp.rs code :) What I noticed, though, was that I was actually asked to enter my admin PIN (presumably to disable the user PIN requirement). We need to be careful with those settings in automated tests.

[robinkrahl]

Could you elaborate on that? If it is about the interactive query, that’s why we have #27. Otherwise I don’t get the problem.

[d-e-s-o]

Ups. Yes, issue #27 is what I am referring to. I must have paged that out already. Thanks for the reminder :)

[d-e-s-o]

@robinkrahl , do you have an opinion on using regular expressions for verifying the output we produce from nitrocli?

For example: 8b7c19f

[robinkrahl]

Looks good, especially as people might want to parse the status output.

[d-e-s-o]

Let me close this issue. I am regarding it as a discussion ground for the general approach to integration testing and we have settled on this, with the first tests having hit devel as part of pull request #54

If we see the need for discussion of individual tests lets create a new issue.