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

Allow importing helper/utility files in test files #1030

Merged

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented May 18, 2023

Work towards: onflow/developer-grants#148

Description

Able to write modular test cases means a developer can import the utility functions from a cadence file to the test file. to avoid repetitive code.

(taken from onflow/developer-grants#148 (comment))

Note: By convention, we consider files that contain the _helper substring, to be helper/utility files. Of course, this is just an initial implementation detail, feel free to suggest an alternative. The main difficulty here is that we can't distinguish from a common.StringLocation, if we are dealing with a contract, or with a simple script containing Cadence code. The importResolver function, has business logic which rejects any imports that are not declared in the flow.json config file.


For contributor use:

  • 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-commenter
Copy link

Codecov Report

Merging #1030 (44d85ad) into master (1b19872) will increase coverage by 0.08%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1030      +/-   ##
==========================================
+ Coverage   39.65%   39.74%   +0.08%     
==========================================
  Files          37       37              
  Lines        1841     1847       +6     
==========================================
+ Hits          730      734       +4     
- Misses       1028     1029       +1     
- Partials       83       84       +1     
Flag Coverage Δ
unittests 39.74% <66.66%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
internal/test/test.go 40.20% <66.66%> (+1.74%) ⬆️

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

Comment on lines 133 to 134
testFiles := make(map[string][]byte, 0)
testFiles[script.Filename] = script.Source
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
testFiles := make(map[string][]byte, 0)
testFiles[script.Filename] = script.Source
testFiles := map[string][]byte{
script.Filename: script.Source,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 6c6d174

relativePath := stringLocation.String()

if strings.Contains(relativePath, "_helper") {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe strip the file extension (path/filepath Ext()) and check the plain filename contains this as a suffix. Or is the idea that this allows both _helper and e.g. _helpers like in the two test files?

Also, maybe refactor this magic string into a constant and add a comment explaining it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or is the idea that this allows both _helper and e.g. _helpers like in the two test files?

Yes, exactly 👍 It might be the case that developers will put everything in a single helper file, or split them in several files. So I wanted to allow some flexibility in both plural & singular names.

Also, maybe refactor this magic string into a constant and add a comment explaining it.

Fixed in e920d18

Copy link
Contributor

@sideninja sideninja 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 after Bastian comments.

@m-Peter m-Peter requested a review from turbolent May 18, 2023 15:34
@sideninja
Copy link
Contributor

@m-Peter can I merge and release?

@sideninja sideninja merged commit 69adf77 into onflow:master May 23, 2023
@m-Peter m-Peter deleted the import-helper-files-in-test-files branch June 5, 2023 16:50
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.

4 participants