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

Fix test matcher #2130

Merged
merged 6 commits into from
Nov 10, 2022
Merged

Conversation

turbolent
Copy link
Member

Depends on #2129

Description

In #2125 I broke the test matcher function, but did not realize at the time, as tests are located in the cadence-tools repo, so only realized when updating the test framework to Stable Cadence.

In #2129 I moved some of the tests to Cadence to avoid similar breakage in the future.
When merged into Stable Cadence, the tests fail, showing the test framework is broken, the creation and invocation of matchers results in erroneous run-time type mismatch errors.

This PR fixes the matcher creation and invocation.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@turbolent turbolent requested a review from a team November 9, 2022 21:19
@turbolent turbolent self-assigned this Nov 9, 2022
Comment on lines -1520 to +1530
matcherTestFunctionType,
matcherTestFieldType,
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix: Create a function that has the type of the Matcher.test field, ((AnyStruct): Bool), so the initialization of the field in Matcher.init works.

Comment on lines -1475 to +1491
staticType, ok := testFunc.StaticType(inter).(interpreter.FunctionStaticType)
if !ok {
typeParameterPair := invocation.TypeParameterTypes.Oldest()
if typeParameterPair == nil {
panic(errors.NewUnreachableError())
}

parameters := staticType.Type.Parameters
parameterType := typeParameterPair.Value
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix: Use the type argument for type parameter T of Test.newMatcher<T>: The wrapper should check if the run-time type of argument passed to the test function (Matcher.test has type ((AnyStruct): Bool)) matches the type argument T

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #2130 (9bbf005) into feature/stable-cadence (2d42211) will increase coverage by 0.18%.
The diff coverage is 43.54%.

@@                    Coverage Diff                     @@
##           feature/stable-cadence    #2130      +/-   ##
==========================================================
+ Coverage                   78.00%   78.19%   +0.18%     
==========================================================
  Files                         306      306              
  Lines                       64158    64209      +51     
==========================================================
+ Hits                        50047    50207     +160     
+ Misses                      12352    12226     -126     
- Partials                     1759     1776      +17     
Flag Coverage Δ
unittests 78.19% <43.54%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/stdlib/test.go 38.12% <43.54%> (+16.25%) ⬆️
runtime/sema/type.go 89.99% <0.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/stable-cadence commit 2d42211
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

old.txtnew.txt
time/opdelta
CheckContractInterfaceFungibleTokenConformance-2119µs ± 1%120µs ± 1%~(p=0.234 n=7+6)
ContractInterfaceFungibleToken-241.3µs ± 1%41.3µs ± 1%~(p=0.628 n=6+7)
InterpretRecursionFib-22.54ms ± 1%2.52ms ± 1%−0.79%(p=0.022 n=6+7)
NewInterpreter/new_interpreter-21.11µs ± 0%1.11µs ± 1%~(p=0.171 n=6+6)
NewInterpreter/new_sub-interpreter-2598ns ± 1%599ns ± 1%~(p=1.000 n=7+6)
ParseArray-28.23ms ± 5%8.11ms ± 1%~(p=0.295 n=7+6)
ParseDeploy/byte_array-212.3ms ± 2%12.6ms ± 6%~(p=0.620 n=7+7)
ParseDeploy/decode_hex-21.21ms ± 1%1.21ms ± 0%~(p=0.429 n=6+5)
ParseFungibleToken/With_memory_metering-2191µs ± 1%192µs ± 1%~(p=0.731 n=7+6)
ParseFungibleToken/Without_memory_metering-2154µs ± 2%154µs ± 4%~(p=0.710 n=7+7)
ParseInfix-27.26µs ± 5%7.13µs ± 0%~(p=0.702 n=7+6)
QualifiedIdentifierCreation/One_level-22.54ns ± 1%2.56ns ± 5%~(p=0.826 n=7+7)
QualifiedIdentifierCreation/Three_levels-2137ns ± 0%138ns ± 2%+1.23%(p=0.008 n=6+7)
RuntimeFungibleTokenTransfer-2726µs ± 4%593µs ±21%−18.33%(p=0.035 n=6+7)
RuntimeResourceDictionaryValues-25.05ms ± 1%5.07ms ± 2%~(p=0.534 n=6+7)
RuntimeScriptNoop-222.4µs ± 4%17.1µs ±35%~(p=0.127 n=6+7)
SuperTypeInference/arrays-2311ns ± 0%311ns ± 0%~(p=0.600 n=7+6)
SuperTypeInference/composites-2131ns ± 0%132ns ± 1%~(p=0.633 n=6+7)
SuperTypeInference/integers-293.3ns ± 0%93.4ns ± 0%~(p=0.078 n=7+7)
ValueIsSubtypeOfSemaType-2104ns ± 1%104ns ± 1%~(p=0.485 n=6+6)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-251.5kB ± 0%51.5kB ± 0%~(all equal)
ContractInterfaceFungibleToken-224.9kB ± 0%24.9kB ± 0%~(p=0.538 n=7+6)
InterpretRecursionFib-21.00MB ± 0%1.00MB ± 0%~(all equal)
NewInterpreter/new_interpreter-2752B ± 0%752B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-2200B ± 0%200B ± 0%~(all equal)
ParseArray-22.74MB ± 3%2.73MB ± 3%~(p=0.620 n=7+7)
ParseDeploy/byte_array-24.21MB ± 3%4.23MB ± 3%~(p=0.535 n=7+7)
ParseDeploy/decode_hex-2214kB ± 0%214kB ± 0%~(p=0.238 n=7+7)
ParseFungibleToken/With_memory_metering-229.8kB ± 0%29.8kB ± 0%~(p=0.808 n=7+7)
ParseFungibleToken/Without_memory_metering-229.8kB ± 0%29.8kB ± 0%~(p=0.393 n=7+7)
ParseInfix-21.93kB ± 0%1.93kB ± 0%~(p=0.709 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
RuntimeFungibleTokenTransfer-2103kB ± 0%102kB ± 1%~(p=0.073 n=6+7)
RuntimeResourceDictionaryValues-22.27MB ± 0%2.27MB ± 0%~(p=0.535 n=7+7)
RuntimeScriptNoop-28.48kB ± 0%8.51kB ± 1%~(p=0.318 n=7+7)
SuperTypeInference/arrays-296.0B ± 0%96.0B ± 0%~(all equal)
SuperTypeInference/composites-20.00B 0.00B ~(all equal)
SuperTypeInference/integers-20.00B 0.00B ~(all equal)
ValueIsSubtypeOfSemaType-248.0B ± 0%48.0B ± 0%~(all equal)
 
allocs/opdelta
CheckContractInterfaceFungibleTokenConformance-2902 ± 0%902 ± 0%~(all equal)
ContractInterfaceFungibleToken-2434 ± 0%434 ± 0%~(all equal)
InterpretRecursionFib-218.9k ± 0%18.9k ± 0%~(all equal)
NewInterpreter/new_interpreter-213.0 ± 0%13.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-24.00 ± 0%4.00 ± 0%~(all equal)
ParseArray-269.6k ± 0%69.6k ± 0%~(p=0.592 n=7+7)
ParseDeploy/byte_array-2104k ± 0%104k ± 0%~(p=0.592 n=7+7)
ParseDeploy/decode_hex-275.0 ± 0%75.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-21.00k ± 0%1.00k ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-21.00k ± 0%1.00k ± 0%~(all equal)
ParseInfix-260.0 ± 0%60.0 ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
RuntimeFungibleTokenTransfer-22.05k ± 0%2.05k ± 0%~(all equal)
RuntimeResourceDictionaryValues-237.0k ± 0%37.0k ± 0%~(p=0.576 n=7+7)
RuntimeScriptNoop-2140 ± 0%140 ± 0%~(all equal)
SuperTypeInference/arrays-23.00 ± 0%3.00 ± 0%~(all equal)
SuperTypeInference/composites-20.00 0.00 ~(all equal)
SuperTypeInference/integers-20.00 0.00 ~(all equal)
ValueIsSubtypeOfSemaType-21.00 ± 0%1.00 ± 0%~(all equal)
 

staticType, ok := testFunc.StaticType(inter).(interpreter.FunctionStaticType)
if !ok {
typeParameterPair := invocation.TypeParameterTypes.Oldest()
if typeParameterPair == nil {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the type parameter is not provided? does it still get resolved and would be not-nil?
e.g: newMatcher(fun (_ value: AnyStruct): Bool { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct!

For example, here's a test case for it https://github.com/onflow/cadence/pull/2130/files#diff-74ad93a1435e307865ff8474567598f410879038788126ec263e27c9244edf98R152-R154 (already existed, was just moved from the test framework to the Cadence repo in #2129)

The invocation gets the types from the elaboration, we keep them there for each invocation, and type parameters bindings are inferred from uses (here, the parameter of the test function expression passed to newMatcher).

@turbolent turbolent merged commit b078580 into feature/stable-cadence Nov 10, 2022
@turbolent turbolent deleted the bastian/fix-test-framework branch November 10, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants