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

Enable coverage reporting for test runners #78

Merged

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Feb 6, 2023

Work towards onflow/developer-grants#132

Complementary PRs:

Description

  • Enable coverage reporting for test runners
  • Include also the test script name, when printing test results for each script.

  • 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

@m-Peter m-Peter changed the title Enable coverage reporting for test runners [WIP] Enable coverage reporting for test runners Feb 6, 2023
@m-Peter m-Peter marked this pull request as draft February 6, 2023 09:27
test/test_runner.go Outdated Show resolved Hide resolved
@m-Peter m-Peter changed the title [WIP] Enable coverage reporting for test runners Enable coverage reporting for test runners Feb 8, 2023
@m-Peter m-Peter marked this pull request as ready for review February 8, 2023 11:23
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!

test/test_framework_test.go Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

Depends on onflow/cadence#2296, so need to wait for a release that includes it

@turbolent turbolent requested a review from a team February 8, 2023 18:22
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.

👌

test/test_runner.go Outdated Show resolved Hide resolved
@m-Peter m-Peter force-pushed the enable-coverage-for-test-runner branch 2 times, most recently from 1bb6b46 to 5de7469 Compare February 11, 2023 13:23
@sideninja
Copy link
Contributor

@m-Peter this CI fails, probably need to update the Cadence.

@m-Peter
Copy link
Contributor Author

m-Peter commented Feb 13, 2023

@sideninja Yes, I think the release flow is probably this:

  1. Release a new cadence version (to include coverage features)
  2. Release a new cadence-tools version (has a dependency on cadence)
  3. Release a new flow-cli version (has a dependency on both cadence & cadence-tools

@sideninja
Copy link
Contributor

@turbolent

Release a new cadence version (to include coverage features)

Do you have any ETA on that?

@m-Peter
Copy link
Contributor Author

m-Peter commented Feb 13, 2023

I was also thinking of randomizing test execution here, with something like:

functions := make(
    []*ast.FunctionDeclaration,
    len(program.Program.FunctionDeclarations()),
)
copy(functions, program.Program.FunctionDeclarations())
rand.Shuffle(len(functions), func(i, j int) {
    functions[i], functions[j] = functions[j], functions[i]
})

for _, funcDecl := range functions {
    ...
}

What are your thoughts on this?

@sideninja
Copy link
Contributor

I was also thinking of randomizing test execution here, with something like:

functions := make(
    []*ast.FunctionDeclaration,
    len(program.Program.FunctionDeclarations()),
)
copy(functions, program.Program.FunctionDeclarations())
rand.Shuffle(len(functions), func(i, j int) {
    functions[i], functions[j] = functions[j], functions[i]
})

for _, funcDecl := range functions {
    ...
}

What are your thoughts on this?

Hmm I'm not sure how I feel about tests not being deterministic, I can see how you could expose flakiness but at the same time it will be hard to identify the source of it I guess due to different execution.

@turbolent
Copy link
Member

@sideninja The plan is to cut a Cadence release today/early this week

@m-Peter
Copy link
Contributor Author

m-Peter commented Feb 14, 2023

The code coverage seems to be working quite well, with the unit testing functionality provided by the Cadence testing framework.
However, when I used the integration testing functionality, I realized that deployed contracts are "hidden" behind a proxy (I.Test). I have created a gist to showcase that.

After that, I was toying around with the integration testing functionality, and the Test contract. By taking most of the tests from test_framework_test.go, I was able to calculate its code coverage 😅 We could probably utilize this somehow. It was fun anyway, to learn its API.

@m-Peter m-Peter force-pushed the enable-coverage-for-test-runner branch from 4706ae1 to ca0e117 Compare February 14, 2023 14:24
@m-Peter
Copy link
Contributor Author

m-Peter commented Feb 14, 2023

I was also thinking of randomizing test execution here, with something like:

functions := make(
    []*ast.FunctionDeclaration,
    len(program.Program.FunctionDeclarations()),
)
copy(functions, program.Program.FunctionDeclarations())
rand.Shuffle(len(functions), func(i, j int) {
    functions[i], functions[j] = functions[j], functions[i]
})

for _, funcDecl := range functions {
    ...
}

What are your thoughts on this?

Hmm I'm not sure how I feel about tests not being deterministic, I can see how you could expose flakiness but at the same time it will be hard to identify the source of it I guess due to different execution.

That's fair, it can be obtrusive if it is done automatically. However, in a stateful paradigm, I think it can uncover some edge cases that are hard to manifest otherwise. If the test suite is executed in a randomized order, the random seed will be printed out, and the developer can then provide this seed, in order to reproduce the failures.

@sideninja
Copy link
Contributor

That's fair, it can be obtrusive if it is done automatically. However, in a stateful paradigm, I think it can uncover some edge cases that are hard to manifest otherwise. If the test suite is executed in a randomized order, the random seed will be printed out, and the developer can then provide this seed, in order to reproduce the failures.

Yeah, it has an upside to indicate issues, but to my experience the wish for that is usually left to a developer to either state in the tests as part of the testing framework to randomize the execution (which in this case I don't think it will be possible since I don't believe there's an API for it) or by providing an execution flag, which might be applicable here. I will leave the decision to you, just wanted to raise the concern I saw.

@m-Peter
Copy link
Contributor Author

m-Peter commented Feb 15, 2023

Yeah, it has an upside to indicate issues, but to my experience the wish for that is usually left to a developer to either state in the tests as part of the testing framework to randomize the execution (which in this case I don't think it will be possible since I don't believe there's an API for it) or by providing an execution flag, which might be applicable here. I will leave the decision to you, just wanted to raise the concern I saw.

Makes sense 👍 I was also thinking about an execution flag as well, to allow developers to specify a randomized execution order. Let's wait and see what Bastian and Supun think about it also.

@m-Peter m-Peter force-pushed the enable-coverage-for-test-runner branch from ca0e117 to cb43a44 Compare March 10, 2023 16:41
@sideninja
Copy link
Contributor

Was #2296 already released? is this still blocked by that?

@m-Peter
Copy link
Contributor Author

m-Peter commented Mar 13, 2023

@sideninja It was first included in this release: https://github.com/onflow/cadence/releases/tag/v0.34.0 . Let me see if I can get it to work with that version.

@m-Peter
Copy link
Contributor Author

m-Peter commented Mar 13, 2023

@sideninja Bumping only the cadence version, doesn't seem to work. We have to also wait for a release of flow-go that includes cadence >= v0.34.0. I see on master branch that it was updated to v0.36.0 (onflow/flow-go@4cd7028). At least, that's my understanding 😅

@m-Peter m-Peter force-pushed the enable-coverage-for-test-runner branch from cb43a44 to 888973b Compare March 13, 2023 10:29
@turbolent
Copy link
Member

Yes, flow-go has been updated. The Emulator was just updated in onflow/flow-emulator#330.

The Emulator will need a couple more improvements (see PR feedback), then after it has been released, I think @dsainati1 will continue updating the downstream dependencies, including the tools here.

@turbolent
Copy link
Member

#86 should unblock this PR

@m-Peter
Copy link
Contributor Author

m-Peter commented Mar 22, 2023

I will give it a shot! Thanks for the heads up 🙌

@turbolent turbolent self-assigned this Mar 22, 2023
@turbolent turbolent added the Improvement Technical work without new features, refactoring, improving tests label Mar 22, 2023
Include also the test script name, when printing test results
for each script.
@m-Peter m-Peter force-pushed the enable-coverage-for-test-runner branch from 888973b to 700964e Compare March 22, 2023 20:06
@m-Peter
Copy link
Contributor Author

m-Peter commented Mar 22, 2023

Well done @turbolent 👏 Rebasing on master, after #86 was merged, seems to have made all checks successful.

@turbolent turbolent merged commit 09d3858 into onflow:master Mar 22, 2023
@m-Peter m-Peter deleted the enable-coverage-for-test-runner branch March 23, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Technical work without new features, refactoring, improving tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants