Skip to content

Conversation

@asnare
Copy link
Contributor

@asnare asnare commented Dec 30, 2025

Changes

What does this PR do?

This PR eliminates the use of Path(__file__) to locate file-based resources. In general:

  • In the main code, this is buggy: modern Python should use importlib.resources instead.
  • In tests, it's brittle and difficult to read because it's normally based on relative paths and traversal of many parent directories. It's also unnecessary, because pytest provides a mechanism for locating the project root. (Tests can also use importlib.resources, and probably should, but that's a bigger refactoring: this is a first step.)

Some incidental changes that are introduced:

  • Type hints in a few places.
  • Functional tests against Snowflake, Oracle and Presto have been consolidated into a single module. (The Snowflake functional tests have been moved into test_snowflake.py: test_databricks.py was misleading.)
  • The PipelineConfig (and Step) classes have been made immutable. (An unnecessary __post__init() method has also been eliminated.)

Tests

  • existing unit tests
  • existing integration tests

@asnare asnare self-assigned this Dec 30, 2025
@asnare asnare added tech debt design flaws and other cascading effects internal technical pr's not end user facing labels Dec 30, 2025
@github-actions
Copy link

github-actions bot commented Dec 30, 2025

✅ 51/51 passed, 5 flaky, 4m34s total

Flaky tests:

  • 🤪 test_transpiles_informatica_to_sparksql_non_interactive[False] (21.823s)
  • 🤪 test_transpiles_informatica_to_sparksql (25.393s)
  • 🤪 test_transpile_teradata_sql_non_interactive[False] (23.27s)
  • 🤪 test_transpile_teradata_sql_non_interactive[True] (22.4s)
  • 🤪 test_transpile_teradata_sql (7.249s)

Running from acceptance #3293

@codecov
Copy link

codecov bot commented Dec 30, 2025

Codecov Report

❌ Patch coverage is 37.50000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.11%. Comparing base (b6a901f) to head (a9faad1).

Files with missing lines Patch % Lines
.../labs/lakebridge/assessments/dashboards/execute.py 0.00% 13 Missing ⚠️
.../labs/lakebridge/assessments/profiler_validator.py 0.00% 3 Missing ⚠️
...databricks/labs/lakebridge/assessments/profiler.py 0.00% 2 Missing ⚠️
...cks/labs/lakebridge/assessments/profiler_config.py 80.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2210   +/-   ##
=======================================
  Coverage   64.11%   64.11%           
=======================================
  Files         101      101           
  Lines        8675     8676    +1     
  Branches      902      898    -4     
=======================================
+ Hits         5562     5563    +1     
  Misses       2941     2941           
  Partials      172      172           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

In addition, update tests:

 - No more Path(__file__) manipulation.
 - Test pipeline configurations now use paths relative to the test resources.
Note: the path being loaded doesn't seem to exist, so it's possible that this was broken before (and still is).
@asnare asnare marked this pull request as ready for review December 30, 2025 15:38
@asnare asnare requested a review from a team as a code owner December 30, 2025 15:38
Comment on lines +71 to +72
# TODO: Verify this, I don't think it works? (These files are part of the test resources.)
schema_def = resources.files(assessment_resources).joinpath(f"{source_tech}_schema_def.yml")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this worked prior to this PR (or after): this is a pure refactoring.

But it does raise an interesting point: how is this tested?

@sundarshankar89: How can I check this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal technical pr's not end user facing tech debt design flaws and other cascading effects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants