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.

Sign up for GitHub

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

Jump to bottom

Make running individual ssl-test easier #18407

Closed
wants to merge 2 commits into from
Closed

Make running individual ssl-test easier #18407

wants to merge 2 commits into from

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented May 25, 2022

Checklist
  • documentation is added or updated
  • tests are added or updated

@tmshort tmshort added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member branch: 3.0 Merge to openssl-3.0 branch labels May 25, 2022
mattcaswell
mattcaswell approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Awesome!! Why didn't I think of this years ago??

@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label May 25, 2022
beldmit
beldmit previously approved these changes May 25, 2022
View reviewed changes
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 added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 25, 2022
@beldmit beldmit dismissed their stale review May 25, 2022 16:39

Tests are failing

@beldmit beldmit added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels May 25, 2022
@beldmit
Copy link
Member

beldmit commented May 25, 2022

@tmshort Sorry, but it is necessary to adjust the doc file - tests are failing.

@tmshort
Copy link
Contributor Author

tmshort commented May 25, 2022

Yup, I'll fixup the docs.

@tmshort
Copy link
Contributor Author

tmshort commented May 25, 2022

emacs strikes again; it automatically indented...

beldmit
beldmit approved these changes May 25, 2022
View reviewed changes
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 added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 25, 2022
@beldmit
Copy link
Member

beldmit commented May 25, 2022

Are we OK with adding this to 3.0? Formally it's a feature...

@tmshort
Copy link
Contributor Author

tmshort commented May 25, 2022

Are we OK with adding this to 3.0? Formally it's a feature...

It's for testing; it doesn't impact library functionality at all. I put the branch: 3.0 on there.

@mattcaswell mattcaswell removed the branch: 3.0 Merge to openssl-3.0 branch label May 26, 2022
@mattcaswell
Copy link
Member

Sorry, didn't spot the 3.0 label on this. I don't think this can go there. The stable release updates policy says:

New tests and test cases are allowed only as part of a bug fix.

https://www.openssl.org/policies/technical/stable-release-updates.html

I'm still ok with master

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@tmshort
Copy link
Contributor Author

tmshort commented May 26, 2022

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

I'll wait 24 hours after @mattcaswell's approval.

Although, one could argue that it's not new tests, nor new test cases.

@beldmit
Copy link
Member

beldmit commented May 26, 2022

I'd like to have this feature added to 3.0, and can raise an OTC hold if necessary.

@paulidale
Copy link
Contributor

I'd like to see this in 3.0 too.

@mattcaswell
Copy link
Member

I'd like to have this feature added to 3.0, and can raise an OTC hold if necessary.

I'd suggest merging this to master anyway (there is no disagreement there), but then keeping this PR open with an OTC hold to specifically address the 3.0 question.

@t8m t8m added the triaged: feature The issue/pr requests/adds a feature label May 27, 2022
@tmshort
Copy link
Contributor Author

tmshort commented May 27, 2022 •
edited

In a couple hours, I'll merge to master. And keep it open pending OTC.

@tmshort
Copy link
Contributor Author

tmshort commented May 27, 2022

Merged to master.

openssl-machine pushed a commit that referenced this pull request May 27, 2022
@tmshort
Make running individual ssl-test easier
eec204f 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18407)
@beldmit beldmit added the hold: need otc decision The OTC needs to make a decision label May 27, 2022
@beldmit
Copy link
Member

beldmit commented May 27, 2022

Question to OTC:
1. Do we allow this PR to be merged to 3.0?
2. Do we want to adjust our policy and allow test framework features to be merged without OTC decision?

@t-j-h
Copy link
Member

t-j-h commented May 31, 2022

OTC: vote in progress on backfit for 3.0

@romen romen mentioned this pull request May 31, 2022
Closed
@tmshort
Copy link
Contributor Author

tmshort commented Jun 7, 2022

The vote did not pass. Since this has already been merged to master, I am closing this PR.

@tmshort tmshort closed this Jun 7, 2022
@tmshort tmshort deleted the ssl-test branch August 1, 2022 17:25
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 14, 2022
@tmshort @t8m
Make running individual ssl-test easier
6df8e97 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18407)

(cherry picked from commit eec204f)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2022
@tmshort @t8m
Make running individual ssl-test easier
e373c08 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18407)

(cherry picked from commit eec204f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@beldmit beldmit beldmit approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch hold: need otc decision The OTC needs to make a decision triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@tmshort @beldmit @mattcaswell @openssl-machine @paulidale @t-j-h @levitte @t8m