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

Refine API of the testing framework #2783

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Sep 14, 2023

Description

  • Remove the Test.newEmulatorBlockchain() method, since EmulatorBackend is now a singleton
  • Remove the Test.useConfiguration() method, since the mappings will be provided by flow.json config file
  • Introduce the Test.getAccount(_ address: Address): Test.Account method, to retrieve an account from the EmulatorBackend
  • Change Test.deployContract to require only name / path & arguments

These changes will allow testing providers to unify unit & integration tests.

An example of unit testing:

import Test
import FooContract from "../contracts/FooContract.cdc"

pub fun setup() {
    let err = Test.deployContract(
        name: "FooContract",
        path: "../contracts/FooContract.cdc",
        arguments: []
    )

    Test.expect(err, Test.beNil())
}

pub fun testGetIntegerTrait() {
    // Arrange
    let testInputs: {Int: String} = {
        -1: "Negative",
        0: "Zero",
        9: "Small"
    }

    for input in testInputs.keys {
        // Act
        let result = FooContract.getIntegerTrait(input)

        // Assert
        Test.assertEqual(result, testInputs[input]!)
    }
}

pub fun testAddSpecialNumber() {
    // Act
    FooContract.addSpecialNumber(78557, "Sierpinski")

    // Assert
    Test.assertEqual("Sierpinski", FooContract.getIntegerTrait(78557))
}

An example of integration testing:

import Test

pub let account = Test.getAccount(0x0000000000000011)

pub fun setup() {
    let err = Test.deployContract(
        name: "FooContract",
        path: "../contracts/FooContract.cdc",
        arguments: []
    )

    Test.expect(err, Test.beNil())
}

pub fun testGetIntegerTrait() {
    let script = Test.readFile("../scripts/get_integer_traits.cdc")
    let result = Test.executeScript(script, [])

    Test.expect(result, Test.beSucceeded())
    Test.assert(result.returnValue! as! Bool)
}

pub fun testAddSpecialNumber() {
    let code = Test.readFile("../transactions/add_special_number.cdc")
    let tx = Test.Transaction(
        code: code,
        authorizers: [account.address],
        signers: [account],
        arguments: [78557, "Sierpinski"]
    )

    let result = Test.executeTransaction(tx)
    Test.expect(result, Test.beSucceeded())
}

All contracts are to be deployed on a blockchain.
In unit tests, developers can import the deployed contracts and directly access fields and call methods.
In integration tests, developers can still import the deployed contracts, to make use of the defined nested types, and interact with the contract by re-using their scripts & transactions.

To make this possible, developers will need to make use of flow.json config file, e.g:

"FooContract": {
      "source": "contracts/FooContract.cdc",
      "aliases": {
        "testing": "0x0000000000000007"
      }
    }

The account where the contracts should be deployed, is set above.


  • 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

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage: 96.42% and project coverage change: +0.10% 🎉

Comparison is base (1309c1d) 79.26% compared to head (20a58ec) 79.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2783      +/-   ##
==========================================
+ Coverage   79.26%   79.37%   +0.10%     
==========================================
  Files         333      333              
  Lines       78769    78696      -73     
==========================================
+ Hits        62440    62468      +28     
+ Misses      14025    13924     -101     
  Partials     2304     2304              
Flag Coverage Δ
unittests 79.37% <96.42%> (+0.10%) ⬆️

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

Files Changed Coverage Δ
runtime/stdlib/test.go 39.79% <ø> (+3.06%) ⬆️
runtime/stdlib/test_emulatorbackend.go 71.37% <95.23%> (+13.10%) ⬆️
runtime/sema/type.go 90.08% <100.00%> (+0.01%) ⬆️
runtime/stdlib/test_contract.go 88.53% <100.00%> (+0.56%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SupunS
Copy link
Member

SupunS commented Sep 14, 2023

Great work @m-Peter!

I haven't looked into the code in detail yet, but I have one suggestion on the API: Can we completely hide the "blockchain" from the users?
For e.g:

import Test 

 // Every function call to `Test.xyz` will use the singleton blockchain instance 

pub let account = Test.getAccount(0x0000000000000011)

pub fun setup() {
    let err = Test.deployContract(     // e.g: Will deploy to the singleton blockchain instance 
        name: "FooContract",
        path: "../contracts/FooContract.cdc",
        arguments: []
    )

    Test.expect(err, Test.beNil())
}

pub fun testGetIntegerTrait() {
    let script = Test.readFile("../scripts/get_integer_traits.cdc")
    let result = Test.executeScript(script, [])
    ...
}

pub fun testAddSpecialNumber() {
    ...
    let tx = Test.Transaction(...)
    let result = Test.executeTransaction(tx)
    ...
}

@m-Peter
Copy link
Contributor Author

m-Peter commented Sep 14, 2023

@SupunS I can try to see how much effort that would require 👍 . In fact, I first went for this approach, since you had already mentioned this API, but fell into some blockers. The Test contract has both a .cdc source file, which is first checked, and then enriched with natively implemented functions. I wanted to do something like:

pub contract Test {
    pub let backend: AnyStruct{BlockchainBackend}

    init(backend: AnyStruct{BlockchainBackend}) {
        self.backend = backend
    }

    pub fun addTransaction(_ tx: Transaction) {
        self.backend.addTransaction(tx)
    }
}

But that resulted in checking errors, so I went for the alternative 😇

To be honest, to me it feels more intuitive to have the Test contract expose functions for assertions and other miscellaneous tasks, while the Test.blockchain singleton, exposes all the methods for interacting with the blockchain.

Test.deployContract(...)
Test.getAccount(...)
Test.executeScript(...)
Test.executeTransaction(...)

vs

blockchain.deployContract(...)
blockchain.getAccount(...)
blockchain.executeScript(...)
blockchain.executeTransaction(...)

Not a very strong opinion though 😅

@SupunS
Copy link
Member

SupunS commented Sep 14, 2023

Yeah, but then again, since there will be one blockchain anyway, having to work with a blockchain field/variable is kind of redundant. Removing that would bring us even closer to unifying unit tests vs integration tests (and simplifying things with that).

Also, with this, it might be possible to replace (Test/blockchain).getAccount() with the getAccount() builtin function eventually, and solve #2726 in the process. (of course, we don't have to do the getAccount API change now, just pointing out that this will make it easier in future)

@m-Peter
Copy link
Contributor Author

m-Peter commented Sep 15, 2023

@SupunS I have removed entirely the Test.Blockchain struct 🙌 .
During the process, I think I have come across to a possible bug in initializeMemberResolvers.
Can you double check this commit: 0f9ba5d ?
That caching seems to not work well for contracts that are partly written in Cadence, and partly with natively implemented functions/fields.
I am unsure as to what is the proper solution for this, so feel free to add a commit and fix this accordingly 🙏

@m-Peter m-Peter force-pushed the refine-test-framework-api branch 2 times, most recently from 533e628 to 0f9ba5d Compare September 15, 2023 13:16
@SupunS
Copy link
Member

SupunS commented Sep 15, 2023

Nice! I'll have a look 👍

Thank you for making all these changes 🙌

@m-Peter
Copy link
Contributor Author

m-Peter commented Sep 19, 2023

I have also removed the Test.useConfiguration() method in 09498dd, since moving forward we'll be using the flow.json config file.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice! Just wondering about the CompositeType member resolution/initialization changes

runtime/sema/type.go Show resolved Hide resolved
runtime/stdlib/contracts/test.cdc Outdated Show resolved Hide resolved
runtime/stdlib/contracts/test.cdc Outdated Show resolved Hide resolved
runtime/stdlib/test_test.go Show resolved Hide resolved
runtime/sema/type.go Show resolved Hide resolved
These changes will allow testing providers to unify
unit & integration tests.
For contracts that contain both a .cdc source file, with Cadence code,
and with some functions being implemented natively, the GetMembers()
does not return the natively implemented members.
@m-Peter
Copy link
Contributor Author

m-Peter commented Sep 25, 2023

I have also renamed the TestFramework.NewEmulatorBackend method to TestFramework.EmulatorBackend, since there will be only one.

@m-Peter m-Peter requested a review from SupunS September 25, 2023 10:55
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Looks good!

@SupunS SupunS merged commit 368d7e3 into onflow:master Sep 27, 2023
11 of 14 checks passed
@m-Peter m-Peter deleted the refine-test-framework-api branch January 12, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants