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

Highlight field errors in generation when they happen #58

Closed
wants to merge 5 commits into from

Conversation

endorama
Copy link
Member

@endorama endorama commented Feb 14, 2023

This PR aims at helping template developers in 2 situations:

  1. when using generate on a missing field
  2. when an error occurs when generating data for a field

In case generate() is being used in the template on a missing field
the current behaviour of returning an empty string hides the problem to
the template developer.

As it is not expected for the template to try generating a missing
field, this commit returns a known string highlighting the problem.

In case there in an error in the bind function for generating a value
for a field, instead of returning an empty string, return a fixed known
string that highlight the issue.

@elasticmachine
Copy link

elasticmachine commented Feb 14, 2023

💔 Tests Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-23T11:02:40.016+0000

  • Duration: 3 min 45 sec

Test stats 🧪

Test Results
Failed 1
Passed 65
Skipped 0
Total 66

Test errors 1

Expand to view the tests failures

Test_PanicOnMissingField – github.com/elastic/elastic-integration-corpus-generator-tool/pkg/genlib
    Expand to view the error details

     Failed 
    

    Expand to view the stacktrace

     === RUN   Test_PanicOnMissingField
        generator_with_text_template_test.go:386: with template: {"alpha":"{{generate "alpha"}}"}
        generator_with_text_template_test.go:357: template: generator:1:12: executing "generator" at <generate "alpha">: error calling generate: missing field: 'alpha' (is it present in fields.yml?)
        generator_with_text_template_test.go:390: <nil>
    --- FAIL: Test_PanicOnMissingField (0.00s)
     
    

Steps errors 1

Expand to view the steps failures

Running Go tests
  • Took 0 min 11 sec . View more details here
  • Description: gotestsum --format testname --junitfile junit-report.xml -- -v ./...

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@@ -40,12 +40,12 @@ func NewGeneratorWithTextTemplate(tpl []byte, cfg Config, fields Fields) (*Gener
templateFns["generate"] = func(field string) interface{} {
bindF, ok := fieldMap[field]
if !ok {
return ""
return "<missing field>"
Copy link
Contributor

@aspacca aspacca Feb 15, 2023

Choose a reason for hiding this comment

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

I was thinking about this, I'd rather send a null, so if the value is assigned to a variable that's expected to be of another type rather than a string, and further functions are applied the template will most likely fail to render

the best would be if we could avoid any kind of failure in the generate body

value, err := bindF(state, nil) can be indeed refactored without returning any error (see here

we cannot do anything about bindF, ok := fieldMap[field], but maybe let's fail the whole command, since having in a template generate "fieldNotInFieldsYaml" is not a transient error but a bug in the template

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather send a null

Returning nil does not halt execution, just renders an empty template.

but maybe let's fail the whole command,

I tried panicking from generate function, but template.Execute recover from panics (see here, go1.19).

I've not been able to have the panic triggered in generate to escape this recovery, due to this I've found no way to properly halt execution.

value, err := bindF(state, nil)

I will remove the commit from this PR, we will address this in a future update.

Copy link
Contributor

@aspacca aspacca Feb 16, 2023

Choose a reason for hiding this comment

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

I tried panicking from generate function, but template.Execute recover from panics (see here, go1.19).

I've not been able to have the panic triggered in generate to escape this recovery, due to this I've found no way to properly halt execution.

we can have an error channel and close it on !ok, then in the emit() we add a select with <-errorChan after parsing the template, that returns an error, and a default that return nil

Copy link
Contributor

Choose a reason for hiding this comment

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

we can have an error channel and close it on !ok, then in the emit() we add a select with <-errorChan after parsing the template, that returns an error, and a default that return nil

please, @endorama , check my proposal: 2e1bc5e#diff-0fc87380e8715fdb1a33c600e6513325ba82801e849851c5856064b7beac59b8R120

In case `generate()` is being used in the template on a missing field
the current behaviour of returning an empty string hides the problem to
the template developer.

As it is not expected for the template to try generating a missing
field, this commit returns a known string highlighting the problem.
In case there in an error in the bind function for generating a value
for a field, instead of returning an empty string, return a fixed known
string that highlight the issue.
@endorama endorama force-pushed the template-missing-fields branch from 69c05e6 to 3aa3d48 Compare February 16, 2023 10:29
@endorama endorama marked this pull request as draft February 16, 2023 10:29
@endorama
Copy link
Member Author

Closing this as the requested improvements are part of #51

@endorama endorama closed this Feb 28, 2023
@endorama endorama deleted the template-missing-fields branch February 28, 2023 11:11
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.

3 participants