Implement all parametrization-related changes suggested in the TODOs
Marco Ricci

Marco Ricci commited on 2025-02-02 11:55:25
Zeige 3 geänderte Dateien mit 48 Einfügungen und 54 Löschungen.


Most of these are straight-forward cases of renaming, consolidating, or
yielding additional parameters.

Regarding the TODOs in the `ssh_agent` test module dealing with `skip`
and `xfail` messages, I decided to follow [Paul Ganssle's use of
`xfail`][GANSSLE_XFAIL]: "use `skip` for tests that *aren't supposed to
work*", and "use `xfail` for tests that *should, but currently don't
pass*".  (In other words, for `skip`, the failure is external to the
system under test and cannot be accomodated for; for `xfail`, it is
internal, or part of the system under test's job to accomodate for it.)
For the aforementioned tests for the `ssh_agent` module, both `skip`
messages occur because of unsatisfied assumptions on the running SSH
agent (a test key isn't loaded, or one kind of failure mode doesn't
occur with this agent), so they shall both remain `skip` messages.

[GANSSLE_XFAIL]: https://blog.ganssle.io/articles/2021/11/pytest-xfail.html
... ...
@@ -400,7 +400,6 @@ class Parametrize(types.SimpleNamespace):
400 400
             ),
401 401
         ],
402 402
     )
403
-    # TODO(the-13th-letter): Add "configure service passphrase" example.
404 403
     UNICODE_NORMALIZATION_COMMAND_LINES = pytest.mark.parametrize(
405 404
         'command_line',
406 405
         [
... ...
@@ -408,6 +407,10 @@ class Parametrize(types.SimpleNamespace):
408 407
                 ['--config', '--phrase'],
409 408
                 id='configure global passphrase',
410 409
             ),
410
+            pytest.param(
411
+                ['--config', '--phrase', '--', 'DUMMY_SERVICE'],
412
+                id='configure service passphrase',
413
+            ),
411 414
             pytest.param(
412 415
                 ['--phrase', '--', DUMMY_SERVICE],
413 416
                 id='interactive passphrase',
... ...
@@ -44,9 +44,8 @@ class Parametrize(types.SimpleNamespace):
44 44
     BAD_CONFIG = pytest.mark.parametrize(
45 45
         'config', ['xxx', 'null', '{"version": 255}']
46 46
     )
47
-    # TODO(the-13th-letter): Rename "result" to "config_data".
48 47
     VAULT_NATIVE_CONFIG_DATA = pytest.mark.parametrize(
49
-        ['config', 'format', 'result'],
48
+        ['config', 'format', 'config_data'],
50 49
         [
51 50
             pytest.param(
52 51
                 tests.VAULT_V02_CONFIG,
... ...
@@ -131,20 +130,18 @@ class Parametrize(types.SimpleNamespace):
131 130
             ),
132 131
         ],
133 132
     )
134
-    # TODO(the-13th-letter): Reorder and rename to "config", "parser_class",
135
-    # "config_data".
136 133
     VAULT_NATIVE_PARSER_CLASS_DATA = pytest.mark.parametrize(
137
-        ['parser_class', 'config', 'result'],
134
+        ['config', 'parser_class', 'config_data'],
138 135
         [
139 136
             pytest.param(
140
-                vault_native.VaultNativeV02ConfigParser,
141 137
                 tests.VAULT_V02_CONFIG,
138
+                vault_native.VaultNativeV02ConfigParser,
142 139
                 tests.VAULT_V02_CONFIG_DATA,
143 140
                 id='0.2',
144 141
             ),
145 142
             pytest.param(
146
-                vault_native.VaultNativeV03ConfigParser,
147 143
                 tests.VAULT_V03_CONFIG,
144
+                vault_native.VaultNativeV03ConfigParser,
148 145
                 tests.VAULT_V03_CONFIG_DATA,
149 146
                 id='0.3',
150 147
             ),
... ...
@@ -238,8 +235,8 @@ class TestCLI:
238 235
     @tests.Parametrize.VAULT_CONFIG_FORMATS_DATA
239 236
     def test_210_load_vault_v02_v03_storeroom(
240 237
         self,
241
-        format: str,
242 238
         config: str | bytes,
239
+        format: str,
243 240
         config_data: dict[str, Any],
244 241
     ) -> None:
245 242
         """Passing a specific format works.
... ...
@@ -691,7 +688,7 @@ class TestVaultNativeConfig:
691 688
         self,
692 689
         config: str,
693 690
         format: Literal['v0.2', 'v0.3'],
694
-        result: _types.VaultConfig | type[Exception],
691
+        config_data: _types.VaultConfig | type[Exception],
695 692
         handler: exporter.ExportVaultConfigDataFunction,
696 693
     ) -> None:
697 694
         """Accept data only of the correct version.
... ...
@@ -718,12 +715,12 @@ class TestVaultNativeConfig:
718 715
                     vault_key=tests.VAULT_MASTER_KEY,
719 716
                 )
720 717
             )
721
-            if isinstance(result, type):
722
-                with pytest.raises(result):
718
+            if isinstance(config_data, type):
719
+                with pytest.raises(config_data):
723 720
                     handler(None, format=format)
724 721
             else:
725 722
                 parsed_config = handler(None, format=format)
726
-                assert parsed_config == result
723
+                assert parsed_config == config_data
727 724
 
728 725
     @Parametrize.PATH
729 726
     @Parametrize.KEY_FORMATS
... ...
@@ -762,9 +759,9 @@ class TestVaultNativeConfig:
762 759
     @Parametrize.VAULT_NATIVE_PARSER_CLASS_DATA
763 760
     def test_300_result_caching(
764 761
         self,
765
-        parser_class: type[vault_native.VaultNativeConfigParser],
766 762
         config: str,
767
-        result: dict[str, Any],
763
+        parser_class: type[vault_native.VaultNativeConfigParser],
764
+        config_data: dict[str, Any],
768 765
     ) -> None:
769 766
         """Cache the results of decrypting/decoding a configuration."""
770 767
 
... ...
@@ -791,7 +788,7 @@ class TestVaultNativeConfig:
791 788
             parser = parser_class(
792 789
                 base64.b64decode(config), tests.VAULT_MASTER_KEY
793 790
             )
794
-            assert parser() == result
791
+            assert parser() == config_data
795 792
             # Now stub out all functions used to calculate the above result.
796 793
             monkeypatch.setattr(
797 794
                 parser, '_parse_contents', null_func('_parse_contents')
... ...
@@ -805,9 +802,9 @@ class TestVaultNativeConfig:
805 802
             monkeypatch.setattr(
806 803
                 parser, '_decrypt_payload', null_func('_decrypt_payload')
807 804
             )
808
-            assert parser() == result
805
+            assert parser() == config_data
809 806
             super_call = vault_native.VaultNativeConfigParser.__call__
810
-            assert super_call(parser) == result
807
+            assert super_call(parser) == config_data
811 808
 
812 809
     def test_400_no_password(self) -> None:
813 810
         """Fail on empty master keys/master passphrases."""
... ...
@@ -219,14 +219,9 @@ class Parametrize(types.SimpleNamespace):
219 219
             ),
220 220
         ],
221 221
     )
222
-    # TODO(the-13th-letter): Modify receiver to receive the whole struct
223
-    # directly.
224 222
     PUBLIC_KEY_DATA = pytest.mark.parametrize(
225
-        ['public_key', 'public_key_data'],
226
-        [
227
-            (val.public_key, val.public_key_data)
228
-            for val in tests.SUPPORTED_KEYS.values()
229
-        ],
223
+        'public_key_struct',
224
+        list(tests.SUPPORTED_KEYS.values()),
230 225
         ids=list(tests.SUPPORTED_KEYS.keys()),
231 226
     )
232 227
     REQUEST_ERROR_RESPONSES = pytest.mark.parametrize(
... ...
@@ -293,21 +288,18 @@ class Parametrize(types.SimpleNamespace):
293 288
             ),
294 289
         ],
295 290
     )
296
-    # TODO(the-13th-letter): Also yield the key type, for reporting purposes.
297 291
     SUPPORTED_SSH_TEST_KEYS = pytest.mark.parametrize(
298
-        'ssh_test_key',
299
-        list(tests.SUPPORTED_KEYS.values()),
292
+        ['ssh_test_key_type', 'ssh_test_key'],
293
+        list(tests.SUPPORTED_KEYS.items()),
300 294
         ids=tests.SUPPORTED_KEYS.keys(),
301 295
     )
302
-    # TODO(the-13th-letter): Also yield the key type, for reporting purposes.
303 296
     UNSUITABLE_SSH_TEST_KEYS = pytest.mark.parametrize(
304
-        'ssh_test_key',
305
-        list(tests.UNSUITABLE_KEYS.values()),
297
+        ['ssh_test_key_type', 'ssh_test_key'],
298
+        list(tests.UNSUITABLE_KEYS.items()),
306 299
         ids=tests.UNSUITABLE_KEYS.keys(),
307 300
     )
308
-    # TODO(the-13th-letter): Rename "value" to "input".
309 301
     UINT32_EXCEPTIONS = pytest.mark.parametrize(
310
-        ['value', 'exc_type', 'exc_pattern'],
302
+        ['input', 'exc_type', 'exc_pattern'],
311 303
         [
312 304
             pytest.param(
313 305
                 10000000000000000,
... ...
@@ -375,15 +367,16 @@ class TestStaticFunctionality:
375 367
 
376 368
     # TODO(the-13th-letter): Re-evaluate if this check is worth keeping.
377 369
     # It cannot provide true tamper-resistence, but probably appears to.
378
-    # TODO(the-13th-letter): Modify parametrization to work directly on the
379
-    # struct.
380 370
     @Parametrize.PUBLIC_KEY_DATA
381 371
     def test_100_key_decoding(
382
-        self, public_key: bytes, public_key_data: bytes
372
+        self,
373
+        public_key_struct: tests.SSHTestKey,
383 374
     ) -> None:
384 375
         """The [`tests.ALL_KEYS`][] public key data looks sane."""
385
-        keydata = base64.b64decode(public_key.split(None, 2)[1])
386
-        assert keydata == public_key_data, (
376
+        keydata = base64.b64decode(
377
+            public_key_struct.public_key.split(None, 2)[1]
378
+        )
379
+        assert keydata == public_key_struct.public_key_data, (
387 380
             "recorded public key data doesn't match"
388 381
         )
389 382
 
... ...
@@ -520,15 +513,14 @@ class TestStaticFunctionality:
520 513
                 assert canon1(encoded) == canon2(encoded)
521 514
                 assert canon1(canon2(encoded)) == canon1(encoded)
522 515
 
523
-    # TODO(the-13th-letter): Rename "value" to "input".
524 516
     @Parametrize.UINT32_EXCEPTIONS
525 517
     def test_310_uint32_exceptions(
526
-        self, value: int, exc_type: type[Exception], exc_pattern: str
518
+        self, input: int, exc_type: type[Exception], exc_pattern: str
527 519
     ) -> None:
528 520
         """`uint32` encoding fails for out-of-bound values."""
529 521
         uint32 = ssh_agent.SSHAgentClient.uint32
530 522
         with pytest.raises(exc_type, match=exc_pattern):
531
-            uint32(value)
523
+            uint32(input)
532 524
 
533 525
     @Parametrize.SSH_STRING_EXCEPTIONS
534 526
     def test_311_string_exceptions(
... ...
@@ -563,13 +555,11 @@ class TestStaticFunctionality:
563 555
 class TestAgentInteraction:
564 556
     """Test actually talking to the SSH agent."""
565 557
 
566
-    # TODO(the-13th-letter): Convert skip into xfail, and include the
567
-    # key type in the skip/xfail message.  This means the key type needs
568
-    # to be passed to the test function as well.
569 558
     @Parametrize.SUPPORTED_SSH_TEST_KEYS
570 559
     def test_200_sign_data_via_agent(
571 560
         self,
572 561
         ssh_agent_client_with_test_keys_loaded: ssh_agent.SSHAgentClient,
562
+        ssh_test_key_type: str,
573 563
         ssh_test_key: tests.SSHTestKey,
574 564
     ) -> None:
575 565
         """Signing data with specific SSH keys works.
... ...
@@ -587,27 +577,29 @@ class TestAgentInteraction:
587 577
         assert expected_signature is not None
588 578
         assert derived_passphrase is not None
589 579
         if public_key_data not in key_comment_pairs:  # pragma: no cover
590
-            pytest.skip('prerequisite SSH key not loaded')
580
+            pytest.skip(f'prerequisite {ssh_test_key_type} SSH key not loaded')
591 581
         signature = bytes(
592 582
             client.sign(payload=vault.Vault.UUID, key=public_key_data)
593 583
         )
594
-        assert signature == expected_signature, 'SSH signature mismatch'
584
+        assert signature == expected_signature, (
585
+            f'SSH signature mismatch ({ssh_test_key_type})'
586
+        )
595 587
         signature2 = bytes(
596 588
             client.sign(payload=vault.Vault.UUID, key=public_key_data)
597 589
         )
598
-        assert signature2 == expected_signature, 'SSH signature mismatch'
590
+        assert signature2 == expected_signature, (
591
+            f'SSH signature mismatch ({ssh_test_key_type})'
592
+        )
599 593
         assert (
600 594
             vault.Vault.phrase_from_key(public_key_data, conn=client)
601 595
             == derived_passphrase
602
-        ), 'SSH signature mismatch'
596
+        ), f'SSH signature mismatch ({ssh_test_key_type})'
603 597
 
604
-    # TODO(the-13th-letter): Include the key type in the skip message.
605
-    # This means the key type needs to be passed to the test function as
606
-    # well.
607 598
     @Parametrize.UNSUITABLE_SSH_TEST_KEYS
608 599
     def test_201_sign_data_via_agent_unsupported(
609 600
         self,
610 601
         ssh_agent_client_with_test_keys_loaded: ssh_agent.SSHAgentClient,
602
+        ssh_test_key_type: str,
611 603
         ssh_test_key: tests.SSHTestKey,
612 604
     ) -> None:
613 605
         """Using an unsuitable key with [`vault.Vault`][] fails.
... ...
@@ -623,12 +615,14 @@ class TestAgentInteraction:
623 615
         key_comment_pairs = {bytes(k): bytes(c) for k, c in client.list_keys()}
624 616
         public_key_data = ssh_test_key.public_key_data
625 617
         if public_key_data not in key_comment_pairs:  # pragma: no cover
626
-            pytest.skip('prerequisite SSH key not loaded')
618
+            pytest.skip(f'prerequisite {ssh_test_key_type} SSH key not loaded')
627 619
         assert not vault.Vault.is_suitable_ssh_key(
628 620
             public_key_data, client=None
629
-        ), 'Expected key to be unsuitable in general'
621
+        ), f'Expected {ssh_test_key_type} key to be unsuitable in general'
630 622
         if vault.Vault.is_suitable_ssh_key(public_key_data, client=client):
631
-            pytest.skip('agent automatically ensures key is suitable')
623
+            pytest.skip(
624
+                f'agent automatically ensures {ssh_test_key_type} key is suitable'
625
+            )
632 626
         with pytest.raises(ValueError, match='unsuitable SSH key'):
633 627
             vault.Vault.phrase_from_key(public_key_data, conn=client)
634 628
 
635 629