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

Add tests for OP_EQUALVERIFY #317

Merged
merged 7 commits into from Aug 9, 2018
Merged

Add tests for OP_EQUALVERIFY #317

merged 7 commits into from Aug 9, 2018

Conversation

s2mr
Copy link

@s2mr s2mr commented Aug 7, 2018

close #279
I implemented tests for OP_EQUALVERIFY.
Would you review this?

@codecov-io
Copy link

codecov-io commented Aug 7, 2018

Codecov Report

Merging #317 into master will increase coverage by 0.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   28.29%   28.83%   +0.53%     
==========================================
  Files         106      107       +1     
  Lines       20142    20241      +99     
==========================================
+ Hits         5700     5836     +136     
+ Misses      14442    14405      -37
Impacted Files Coverage Δ
BitcoinCashKit/Core/Scripts/Script.swift 45.67% <0%> (-0.12%) ⬇️
BitcoinCashKit/Core/Scripts/OpCodeFactory.swift 96.42% <0%> (ø) ⬆️
...shKit/Core/Scripts/OP_CODE/Numeric/OP_LSHIFT.swift
BitcoinCashKit/Core/Scripts/OP_CODE/OP_DUP.swift
BitcoinCashKit/Core/Scripts/OP_CODE/OP_N.swift
...nCashKit/Core/Scripts/OP_CODE/Numeric/OP_MAX.swift
...nCashKit/Core/Scripts/OP_CODE/Numeric/OP_DIV.swift
...tcoinCashKit/Core/Scripts/OP_CODE/OP_HASH160.swift
...itcoinCashKit/Core/Scripts/OP_CODE/OP_VERIFY.swift
...CashKit/Core/Scripts/OP_CODE/Numeric/OP_2DIV.swift
... and 79 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfb714b...e572ecf. Read the comment docs.

@@ -126,20 +133,41 @@ class OpCodeTests: XCTestCase {
let dataOnTop: Data = context.stack.last!
try opcode.execute(context)
XCTAssertEqual(context.stack.count, stackCountAtFirst + 1, "\(opcode.name)(\(String(format: "%02x", opcode.value)) test: One data should be added to stack.")
XCTAssertEqual(context.stack.dropLast().map(Data.init), stackSnapShot, "\(opcode.name)(\(String(format: "%02x", opcode.value)) test: The data except the top should be the same after the execution.")
XCTAssertEqual(context.stack.dropLast().map({Data.init($0)}), stackSnapShot, "\(opcode.name)(\(String(format: "%02x", opcode.value)) test: The data except the top should be the same after the execution.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Data($0) is better.
Explicitly calling .init() should be avoided.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change this line?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I committed this line for my wrong.

But my Xcode shows this error.
Ambiguous use of 'init'
My environment is Xcode Version 10.0 beta 5 (10L221o)

What should I do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems the same issue as here.
I think this is because you are using Xcode beta version.
https://bugs.swift.org/browse/SR-3842

Copy link
Collaborator

Choose a reason for hiding this comment

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

As @akifuj said, you'd better write like this.
.map({Data($0)})

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for picking link.
I fixed as .map { Data($0) }

try context.pushToStack(1)
try context.pushToStack(3)
XCTAssertEqual(context.stack.count, 2)
XCTAssertThrowsError(try opcode.execute(context))
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@s2mr
Copy link
Author

s2mr commented Aug 8, 2018

I fixed for the review.

} catch OpCodeExecutionError.error("OP_VERIFY failed.") {
// success
} catch let error {
fail(with: opcode, error: error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part should throw error like this.
"Should throw OpCodeExecutionError .error(\"OP_VERIFY failed.\"), but threw \(error)"

} catch OpCodeExecutionError.opcodeRequiresItemsOnStack(1) {
// success
} catch let error {
fail(with: opcode, error: error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part should throw error like this.
"Should throw OpCodeExecutionError .error(\"OP_VERIFY failed.\"), but threw \(error)"

Copy link
Collaborator

@usatie usatie left a comment

Choose a reason for hiding this comment

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

Please change it.

@s2mr
Copy link
Author

s2mr commented Aug 8, 2018

@usatie Thanks for the review many times.
Please review it.

Copy link
Collaborator

@usatie usatie left a comment

Choose a reason for hiding this comment

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

LGTM

@usatie usatie merged commit 97ee2b0 into BitcoinCashKit:master Aug 9, 2018
@usatie
Copy link
Collaborator

usatie commented Aug 9, 2018

@GitCash send 1000 bits to @kzumu
Thanks!

@GitCash
Copy link

GitCash commented Aug 9, 2018

Hey kzumu, user usatie tipped you 1000 bits in Bitcoin Cash ( ~ $0.591 ).

Click here to claim it!

You can also add the "thumbs down" reaction to usatie's comment above to prevent future tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test OP_EQUALVERIFY
5 participants