-
Notifications
You must be signed in to change notification settings - Fork 2.6k
chore: Refactor pipeline run tests #10451
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
| @then("draw it to file") | ||
| def draw_pipeline(pipeline_data: tuple[Pipeline, list[PipelineRunData]], request: pytest.FixtureRequest) -> None: | ||
| """ | ||
| Draw the pipeline to a file with the same name as the test. | ||
| """ | ||
| if m := PIPELINE_NAME_REGEX.search(request.node.name): | ||
| name = m.group(1).replace(" ", "_") | ||
| pipeline = pipeline_data[0] | ||
| graphs_dir = Path(request.config.rootpath) / "test_pipeline_graphs" | ||
| graphs_dir.mkdir(exist_ok=True) | ||
| pipeline.draw(path=graphs_dir / f"{name}.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this utility function since it wasn't used in the tests.
| [ | ||
| PipelineRunData( | ||
| inputs={ | ||
| "prompt": {"question": "This is a question that has an answer", "columns": columns}, | ||
| "router": {"question": "This is a question that has an answer"}, | ||
| }, | ||
| expected_outputs={"sql_querier": {"results": "This is the query result", "query": "Some SQL query"}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example was removed since it was left over accidentally and already being skipped. It's a copy from a different test.
Pull Request Test Coverage Report for Build 21361008047Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
anakin87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🧹
Related Issues
Proposed Changes:
While working on adding more tests for the auto joiner experiment I found there was many opportunities to reduce duplicate code and code lines in the pipeline run tests.
How did you test it?
Existing tests. Didn't remove or add tests.
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.