feat: Improve robustness of DeviceId::from_str, add tests

[jwagner]

I think it would be better if from_str returned a Err rather than panicing when it encounters an unknown platform.
A real life example where this could happen is when reading a device id from a config file that might have been written by a newer version or on a different platform.

I have also added some tests which then made me discover that webaudioworklet was missing from from_str.
I assume that this wasn't intentional, so this commit adds this back as well.

The added tests aren't in a #[cfg(test)] block because the other tests in this file weren't either, so I presume that is the desired style.

[jwagner]

I can't add reviewers but it would be sweet if @xephyris could have a look, especially regarding the addition of WebAudioWorklet to from_str.

[xephyris]

The addition of webaudioworklet seems to work fine.

One question I do have is are the from_str() tests really necessary in this scenario?

The addition of the tests doesn't do any harm, but they also seem unnecessary to me as they don't particularly test the robustness of the DeviceId system but rather just test the match statement in from_str() which should always be valid except for in the case of a user error.

Just read your description and do see why such a test is valid.

Would it be possible though to make it so that the test uses something like a match statement that would fail in the case where an API has not been included in the test? That way it can easily noticed when testing, and users won't forget to update the test when adding new APIs.

I still think that the test_device_id_from_str, seems somewhat unnecessary, as its functionality seems to be encompassed in the second test, and it should always pass except for in the case of a user error.

I would be happy to hear your thoughts on this issue though.

[jwagner]

Thanks a lot for your prompt feedback @xephyris.

Would it be possible though to make it so that the test uses something like a match statement that would fail in the case where an API has not been included in the test? That way it can easily noticed when testing, and users won't forget to update the test when adding new APIs.

I'll give that a try.

I still think that the test_device_id_from_str, seems somewhat unnecessary, as its functionality seems to be encompassed in the second test, and it should always pass except for in the case of a user error.

This is an embarrassing copy paste mistake. What I wanted is one test function for backwards compatibility and edge cases of from_string and another one testing that the invariant of from_str(x.to_string()) == x holds.

Can't update the PR right now, but I'll get to it in the evening.

[jwagner]

The test_device_id_from_str test should now make a bit more sense.

I haven't found a nice way to ensure that the test covers all the enum variants though. A macro like this to generate a dummy match statement would work but might be overkill. Can you think of a better solution @xephyris ?

[xephyris]

One way we could do this would be with an empty match statement. While this definitely isn't the cleanest method to do things, I do believe it is the simplest and the easiest to understand.

For your demo, something we can do instead of creating a whole new macro could be just like this:

    let test = TestEnum::A;
    // Will produce error because TestEnum::C is missing
    match test {
        TestEnum::A => {},
        TestEnum::B => {}
    }

which produces an error

error[E0004]: non-exhaustive patterns: `TestEnum::C` not covered
  --> src/main.rs:34:11
   |
34 |     match test {
   |           ^^^^ pattern `TestEnum::C` not covered
   |
note: `TestEnum` defined here

allowing us to use the error statements provided by the match statement, which I believe is a cleaner and easier to understand error than this:

error: unexpected end of macro invocation
  --> src/main.rs:28:38
   |
 1 | macro_rules! enum_for {
   | --------------------- when calling this macro
...
28 |         TestEnum::B => println!("b"),
   |                                      ^ missing tokens in macro arguments
   |
note: while trying to match meta-variable `$arm:path`
  --> src/main.rs:2:9
   |
 2 |     ($ ($arm: path => $process: expr ), *) => {{
   |         ^^^^^^^^^^

Do you have any better ideas?

[jwagner]

Like this @xephyris ?

[xephyris]

Yeah this seems to work nicely.

[roderickvd]

#1046 did most of this but not the test. What would you like to do?

[jwagner]

There is a lot less to test now as well. I'll have a quick look, probably tomorrow, and see if any of it still makes sense.

[jwagner]

This is covered by the changes. 👍