Update linter integration tests to ensure notebook source is linted#2853
Merged
Update linter integration tests to ensure notebook source is linted#2853
Conversation
Includes improved verification to ensure it's passing for the right reason.
|
✅ 42/42 passed, 25m58s total Running from acceptance #6538 |
…o their extension.
asnare
changed the title
test_running_real_workflow_linter_job fixture: it wasn't testing what it thought it was
JCZuurmond
reviewed
Oct 8, 2024
Contributor
JCZuurmond
left a comment
JCZuurmond
left a comment
There was a problem hiding this comment.
Has overlapping changes with #2852
This was referenced Oct 9, 2024
JCZuurmond
suggested changes
Oct 9, 2024
Contributor
JCZuurmond
left a comment
JCZuurmond
left a comment
There was a problem hiding this comment.
Some changes to use the latest version of the fixtures, otherwise looks good
| installation_ctx.deployed_workflows.relay_logs("experimental-workflow-linter") | ||
| assert False, "No workflow problems found" | ||
| dfsa_records = installation_ctx.directfs_access_crawler_for_paths.snapshot() | ||
| assert dfsa_records |
Contributor
There was a problem hiding this comment.
just some record? Or a specific amount?
Collaborator
There was a problem hiding this comment.
Suggested change
| assert dfsa_records | |
| assert dfsa_records == [...] |
can we please assert for specific values?
Contributor
Author
There was a problem hiding this comment.
It was intended to just check for the presence of records, like the check above.
I've added a comment (cf544c1) to this effect.
| ), | ||
| notebook_task=jobs.NotebookTask(notebook_path=str(notebook)), | ||
| ) | ||
| j = make_job(tasks=[task]) |
Contributor
There was a problem hiding this comment.
Remove most of the above and use make_job(content=...)
| libraries=[compute.Library(pypi=compute.PythonPyPiLibrary(package="greenlet"))], | ||
| libraries=[compute.Library(pypi=compute.PythonPyPiLibrary(package="dbt-core==1.8.7"))], | ||
| ) | ||
| j = make_job(tasks=[task]) |
Contributor
There was a problem hiding this comment.
Suggested change
| j = make_job(tasks=[task]) | |
| j = make_job(content="import greenlet", task_type=jobs.SparkPythonTask) |
| libraries=[compute.Library(pypi=compute.PythonPyPiLibrary(package="greenlet"))], | ||
| libraries=[compute.Library(pypi=compute.PythonPyPiLibrary(package="dbt-core==1.8.7"))], | ||
| ) | ||
| j = make_job(tasks=[task]) |
nfx
reviewed
Oct 9, 2024
| installation_ctx.deployed_workflows.relay_logs("experimental-workflow-linter") | ||
| assert False, "No workflow problems found" | ||
| dfsa_records = installation_ctx.directfs_access_crawler_for_paths.snapshot() | ||
| assert dfsa_records |
Collaborator
There was a problem hiding this comment.
Suggested change
| assert dfsa_records | |
| assert dfsa_records == [...] |
can we please assert for specific values?
| assert False, "No workflow problems found" | ||
| dfsa_records = installation_ctx.directfs_access_crawler_for_paths.snapshot() | ||
| used_table_records = installation_ctx.used_tables_crawler_for_paths.snapshot() | ||
| assert dfsa_records and used_table_records |
Collaborator
There was a problem hiding this comment.
could we assert for exact values? e.g. assert dfsa_records == [...]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
This PR updates some linting related integration tests that weren't testing what they expected:
Some usage of
greenlethas also been replaced: it was previously unknown to the linter and various integration tests assumed this. Since #2830 these tests have no longer been working correctly.Related Issues
Overlaps with #2852.
Overlaps with #2867.
Overlaps with #2868.
Resolves #2843.
Resolves #2844.
Resolves #2845.
Resolves #2846.
Tests