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

code health - unit tests for crypto/stack. #2924

Closed
wants to merge 1 commit into from
Closed

code health - unit tests for crypto/stack. #2924

wants to merge 1 commit into from

Conversation

paulidale
Copy link
Contributor

@paulidale paulidale commented Mar 14, 2017 •
edited

  • tests are added or updated

A suite of unit tests for the crypto/stack code. The tests aren't exhaustive but cover everything.

@levitte
Copy link
Member

levitte commented Mar 14, 2017

Hi, cool test.

So here, I'll take the opportunity to put you through a learning experience, if you don't mind.

We have a simple test programming framework that we think will make it easier, faster and more consistent to write test programs.

Have a look at test/testutil.h, it has some comments at the beginning quickly explaining the structure of the framework. If you need examples, grep ADD_ test/*.c.

@levitte levitte added code-health branch: master Merge to master branch labels Mar 14, 2017
@levitte levitte self-assigned this Mar 14, 2017
@mattcaswell
Copy link
Member

This is really cool!! :-)

@paulidale
Copy link
Contributor Author

Thanks for the feedback, I'll update the test.
I looked at a couple of the tests that did't use framework.

@levitte
Copy link
Member

levitte commented Mar 15, 2017

I looked at a couple of the tests that did't use framework.

Some of them are age old, others... well, let's just say that we're still on a learning curve ourselves.

@paulidale
Copy link
Contributor Author

I've updated the tests to use the framework -- please let me know if I've done something wrong here.

I also added tests for the remaining macros and functions. They are still a bit shallow in places.

@paulidale paulidale force-pushed the test-stack branch 3 times, most recently from e6424a0 to 81b0f32 Compare March 15, 2017 02:53
@paulidale
Copy link
Contributor Author

Does anyone know why that one build sometimes locks up and times out?

@mattcaswell
Copy link
Member

Does anyone know why that one build sometimes locks up and times out?

Probably #2955

@paulidale
Copy link
Contributor Author

Thanks for pointing me there. The intermittent failures were confusing for a while.

@dot-asm dot-asm added the approval: done This pull request has the required number of approvals label Mar 15, 2017
@levitte
Copy link
Member

levitte commented Mar 15, 2017

Merged. 9837496

@levitte levitte closed this Mar 15, 2017
levitte pushed a commit that referenced this pull request Mar 15, 2017
@paulidale @levitte
Unit tests for crypto/stack.
9837496 
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #2924)
@paulidale paulidale deleted the test-stack branch March 16, 2017 00:24
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

@dot-asm dot-asm dot-asm approved these changes

Assignees

@levitte levitte

Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@paulidale @levitte @mattcaswell @dot-asm