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

TransformExecutor Tests #431

Merged
merged 12 commits into from
Nov 6, 2023
Merged

Conversation

f3l1x98
Copy link
Contributor

@f3l1x98 f3l1x98 commented Aug 28, 2023

Closes #415
Note: This PR also includes #462 due to required functionality!

This PR adds tests for TransformExecutor.

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Aug 28, 2023

Two things I have found:

  1. For the sake of creating a table as simple and fast as possible I added an constructor to the Table class. If I overlooked a reason as to why not do that please comment.
  2. I wanted to add a test that would check if a row has been deleted because the expression could not be evaluated and I discovered that currently this only happens if there is no expression. The reason for that is that the evaluateExpression function throws assertion errors in case there is an issue which are not caught inside the TransformExecutor#doExecuteTransform. Is that intended, or should I add a try catch? (Because the logged error sounds like it should also handle the exceptions)

@georg-schwarz @rhazn

@f3l1x98 f3l1x98 mentioned this pull request Aug 28, 2023
5 tasks
@f3l1x98 f3l1x98 changed the title [WIP] TransformExecutor Tests TransformExecutor Tests Sep 4, 2023
@georg-schwarz
Copy link
Member

Two things I have found:

  1. For the sake of creating a table as simple and fast as possible I added an constructor to the Table class. If I overlooked a reason as to why not do that please comment.
  2. I wanted to add a test that would check if a row has been deleted because the expression could not be evaluated and I discovered that currently this only happens if there is no expression. The reason for that is that the evaluateExpression function throws assertion errors in case there is an issue which are not caught inside the TransformExecutor#doExecuteTransform. Is that intended, or should I add a try catch? (Because the logged error sounds like it should also handle the exceptions)

Could you post a link to the file?
Assertions should never fail, they point to a bug.
If a user led to the mistake, there should be an exception IMO.

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Nov 6, 2023

Two things I have found:

  1. For the sake of creating a table as simple and fast as possible I added an constructor to the Table class. If I overlooked a reason as to why not do that please comment.
  2. I wanted to add a test that would check if a row has been deleted because the expression could not be evaluated and I discovered that currently this only happens if there is no expression. The reason for that is that the evaluateExpression function throws assertion errors in case there is an issue which are not caught inside the TransformExecutor#doExecuteTransform. Is that intended, or should I add a try catch? (Because the logged error sounds like it should also handle the exceptions)

Could you post a link to the file? Assertions should never fail, they point to a bug. If a user led to the mistake, there should be an exception IMO.

I am talking about this commit (seems like I already added the try catch 😅 ): 28b5836

Because without this commit any exceptions thrown inside evaluateExpression will propagate through the whole stack up to the tests (or the system somewhere higher up in production).
Not sure why I wrote assertion errors instead of exceptions in general...

@f3l1x98 f3l1x98 requested a review from georg-schwarz November 6, 2023 10:13
@f3l1x98 f3l1x98 merged commit fdd7c04 into main Nov 6, 2023
2 checks passed
@f3l1x98 f3l1x98 deleted the feature/execution-transform-executor-tests branch November 6, 2023 12:42
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] libs/execution tests
2 participants