Hi,
On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <[email protected]> wrote:
>
> 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.
In practice we try to set values for various which are important, so
future tests should explicitly delete the var if needed. But I am OK
with mocking, instead.
>
> [...]
>
> >>> + '-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?
OK I sent a patch.
Regards,
Simon
