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

Revert addition of ecdsa test #189

Closed
wants to merge 1 commit into from
Closed

Revert addition of ecdsa test #189

wants to merge 1 commit into from

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Aug 1, 2018

Revert the addition of ecdsa test, as it seems that CI still fails.
Resolves #184.

Revert addition of ecdsa test
2dca91a 
Revert the addition of ecdsa test, as it seems that CI still fails.
Resolves ARMmbed#184.
@RonEld RonEld requested review from simonbutcher, AndrzejKurek and k-stachowiak and removed request for AndrzejKurek August 1, 2018 11:03
@andresag01
Copy link

Note that #194 reworks the benchmark example to address this problem (among others)

@AndrzejKurek
Copy link
Contributor

I would also opt for Andres' solution, than for disabling the tests.

@RonEld
Copy link
Contributor Author

RonEld commented Aug 22, 2018

I agree, and when I removed the ECDSA disabling, the tests worked for me, however ,in CI, the tests failed for the same board

k-stachowiak
k-stachowiak approved these changes Aug 27, 2018
View reviewed changes
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

I ran benchmark application from this PR against ARM, GCC_ARM and IAR compiler locally, on my K64F board and I confirm that I don't observe any issues in such setups.

AndrzejKurek
AndrzejKurek approved these changes Aug 28, 2018
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

I always prefer fixing things rather than disabling them, but the PR itself is OK.

@simonbutcher
Copy link
Contributor

retest

@simonbutcher simonbutcher removed their request for review October 15, 2018 14:48
@RonEld
Copy link
Contributor Author

RonEld commented Oct 15, 2018

@sbutcher-arm Do we need this merged, assuming #194 will fix the memory issues?

@simonbutcher
Copy link
Contributor

PR #194 fixes the problem properly, so I don't think we need this PR now. Thanks for supplying it anyway @RonEld.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrzejKurek AndrzejKurek AndrzejKurek approved these changes

@k-stachowiak k-stachowiak k-stachowiak approved these changes

Assignees
No one assigned
Labels
bug needs: CI runCI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RonEld @andresag01 @AndrzejKurek @simonbutcher @k-stachowiak