Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/recipes/80-test_cms.t: Fix the "CAdES ko" test #19731

Closed
wants to merge 1 commit into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Nov 22, 2022

This test had commands that assumes that runner_loop() is used to perform
the tests. These tests still run fine because Unix accepts braces in file
names, but other operating systems might not.

This test had commands that assumes that runner_loop() is used to perform
the tests.  These tests still run fine because Unix accepts braces in file
names, but other operating systems might not.
@levitte levitte added approval: otc review pending This pull request needs review by an OTC member approval: review pending This pull request needs review by a committer branch: 3.1 Merge to openssl-3.1 branch: master Merge to master branch severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels Nov 22, 2022
@levitte levitte self-assigned this Nov 22, 2022
@levitte levitte requested review from a team November 22, 2022 14:10
@levitte
Copy link
Member Author

levitte commented Nov 22, 2022

Made urgent because this test fails on VMS. All other tests are
successful as far as I've been able to determine.

Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@beldmit beldmit removed the approval: otc review pending This pull request needs review by an OTC member label Nov 22, 2022
@beldmit
Copy link
Member

beldmit commented Nov 22, 2022

Agree urgent

@@ -800,6 +800,7 @@ subtest "CAdES ko tests\n" => sub {
SKIP: {
my $skip_reason = check_availability($$_[0]);
skip $skip_reason, 1 if $skip_reason;
1 while unlink "cades-ko.txt";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a loop required here?

Also, doesn't this leave cades-ko.txt undeleted on the final loop? Or, maybe that doesn't matter...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's required on VMS, where files may exist in multiple generations (so cades-ko.txt;1, cades-ko.txt;2, ...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding deleting all but for the final loop, you're right about that. However, the array we use has only one element. The function runner_loop handles that better, but isn't quite suitable for this particular test because of the second negative test. I've thoughts on reworking runner_loop, but that's overkill for this fix.

@t8m t8m added the triaged: bug The issue/pr is/fixes a bug label Nov 22, 2022
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK with urgent

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge tests: exempted The PR is exempt from requirements for testing and removed approval: review pending This pull request needs review by a committer labels Nov 22, 2022
@t8m
Copy link
Member

t8m commented Nov 22, 2022

Merged to 3.1 and master branches. Thank you.

@t8m t8m closed this Nov 22, 2022
openssl-machine pushed a commit that referenced this pull request Nov 22, 2022
This test had commands that assumes that runner_loop() is used to perform
the tests.  These tests still run fine because Unix accepts braces in file
names, but other operating systems might not.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19731)
openssl-machine pushed a commit that referenced this pull request Nov 22, 2022
This test had commands that assumes that runner_loop() is used to perform
the tests.  These tests still run fine because Unix accepts braces in file
names, but other operating systems might not.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19731)

(cherry picked from commit 20d3731)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
This test had commands that assumes that runner_loop() is used to perform
the tests.  These tests still run fine because Unix accepts braces in file
names, but other operating systems might not.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19731)

(cherry picked from commit 20d3731)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.1 Merge to openssl-3.1 severity: urgent Fixes an urgent issue (exempt from 24h grace period) tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants