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

Output more helpful error messages for pipeline runtime errors #629

Merged
merged 11 commits into from
Dec 3, 2020

Conversation

mfelsche
Copy link
Member

@mfelsche mfelsche commented Dec 1, 2020

Pull request

Description

If we have an error that is locatable within the pipeline source, we are going do display an error message, showing the error within the pipeline source.

Also:

  • Added integration tests for hygienic runtime error messages
  • Added possibility to add environment variables in command based tests (turns out it wasnt needed)
  • Put error messages to stderr when using tremor run.

Related

Checklist

  • The RFC, if required, has been submitted and approved
  • Any user-facing impact of the changes is reflected in docs.tremor.rs
  • The code is tested
  • Use of unsafe code is reasoned about in a comment
  • Update CHANGELOG.md appropriately, recording any changes, bug fixes or other observable changes in behavior

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #629 (701ebff) into main (a3715d9) will increase coverage by 0.06%.
The diff coverage is 73.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #629      +/-   ##
==========================================
+ Coverage   80.83%   80.89%   +0.06%     
==========================================
  Files         101      101              
  Lines       14304    14434     +130     
==========================================
+ Hits        11562    11676     +114     
- Misses       2742     2758      +16     
Impacted Files Coverage Δ
src/offramp.rs 44.66% <0.00%> (ø)
tests/script_error.rs 94.11% <ø> (ø)
tremor-pipeline/src/executable_graph.rs 85.84% <ø> (+0.22%) ⬆️
tremor-script/src/lexer.rs 83.13% <67.08%> (-0.57%) ⬇️
src/pipeline.rs 76.33% <82.08%> (+12.18%) ⬆️
tremor-pipeline/src/query.rs 87.37% <100.00%> (+0.03%) ⬆️
tremor-script/src/interpreter.rs 81.28% <0.00%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3715d9...7483454. Read the comment docs.

Licenser
Licenser previously approved these changes Dec 1, 2020
Copy link
Member

@Licenser Licenser left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Matthias Wahl added 6 commits December 2, 2020 09:57
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Licenser
Licenser previously approved these changes Dec 2, 2020
… float literals.

Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Matthias Wahl added 2 commits December 2, 2020 22:00
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Licenser
Licenser previously approved these changes Dec 2, 2020
Copy link
Member

@Licenser Licenser left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Licenser
Copy link
Member

Licenser commented Dec 2, 2020

Looks like -0.1% Coverage :(

@mfelsche
Copy link
Member Author

mfelsche commented Dec 2, 2020

adding some tests tomorrow morning.

@Licenser
Copy link
Member

Licenser commented Dec 2, 2020

Let me switch the state to draft so we know it's pre-review :)

@Licenser Licenser marked this pull request as draft December 2, 2020 22:06
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
@mfelsche mfelsche marked this pull request as ready for review December 3, 2020 10:38
@mfelsche
Copy link
Member Author

mfelsche commented Dec 3, 2020

+0.06% take this codecov! ⚔️

Copy link
Member

@Licenser Licenser left a comment

Choose a reason for hiding this comment

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

LGTM 👍 and <3 for the coverage increase!

@mfelsche
Copy link
Member Author

mfelsche commented Dec 3, 2020

Ehrensache! ❤️

@Licenser
Copy link
Member

Licenser commented Dec 3, 2020

All tests pass and codecov status is reported, will merge since codecov is still broken :/

@Licenser Licenser merged commit 0b4f20a into main Dec 3, 2020
@Licenser Licenser deleted the hygienic-errors-server branch December 3, 2020 17:22
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.

Non hygenic error with bad access in trickle minimal value of i64 can not be expressed in tremor script
2 participants