Hi Wojciech,
On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
Hello Quentin,
Hi Wojciech,
On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
[...]
+ os.environ['SOFTHSM2_CONF'] = softhsm2_conf
This is wrong, you'll be messing up with the environment of all tests being
run in the same thread. You must use the "with
unittest.mock.patch.dict('os.environ'," implementation I used in
testFitSignPKCS11Simple.
Well, I have done so in my V2 but has been commented as wrong by the
first reviewer. I will restore it back.
Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
how we should be doing it.
This is likely fine on its own because there's only one test that is now
modifying os.environ's SOFTHSM2_CONF but this will be a problem next
time a test wants to modify it. I actually hit this issue when
developing the PKCS11 fit signing tests as I had two tests modifying the
environment.
The only trace of it left is the changelog in
https://lore.kernel.org/u-boot/[email protected]/
"""
- fixed issues due to modification of the environment in tests failing
other tests, by using unittest.mock.patch.dict() on os.environ as
suggested by the unittest.mock doc,
"""
and you can check the diff between the v2 and v3 to check I used to
modify the env directly but now mock it instead.
Sorry for not catching this, should have answered to Simon in the v2.
[...]
+ '-CAfile', cert_file, '-no_check_time')
+ except ValueError:
+ self.assertIn('UEFI Capsule verification failed')
+
I don't think this is valid.
https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn
states you need 2 (or three) argumentis, only one's provided here.
I'm assuming we don't need the try..except since the raised Exception will
just stop (and fail) the execution of the test but continue with the other
ones? Can you check (locally) by purposefully using the wrong key for
example?
It has worked (failed) with try/except. I had to add no time check as the
certificate
Thanks for the heads up, @Simon any chance you can update the
certificate so we're fine for the decades to come?
Cheers,
Quentin
