Conversation
fernst
force-pushed
the
test-proto-changes
branch
2 times, most recently
from
512080f to
6f6b3ee
Compare
core/actions/test.ts
Outdated
| this.testTarget = dataform.Target.create(dataset.getTarget()); | ||
|
|
||
| // Set the test query with the fully qualified table references. | ||
| if (dataset instanceof Table) { |
There was a problem hiding this comment.
These if/else branches are equivalent
There was a problem hiding this comment.
True. I've consolidated this code.
kolina
left a comment
kolina
left a comment
There was a problem hiding this comment.
Can you please against the latest main? So integration tests will run with non-expired credentials
fernst
force-pushed
the
test-proto-changes
branch
from
149c2fe to
72903be
Compare
fernst
force-pushed
the
test-proto-changes
branch
from
0dc9788 to
b03d245
Compare
core/actions/test.ts
Outdated
| ); | ||
| } | ||
|
|
||
| private overrideTargetWithNewName(target: dataform.ITarget, testName: string): dataform.Target { |
There was a problem hiding this comment.
Nit: Can be a local function, as it doesn't depend on this.
core/session.ts
Outdated
| const fullyQualifiedDependencies: { [name: string]: dataform.ITarget } = {}; | ||
| if (action instanceof dataform.Declaration || !action.dependencyTargets) { | ||
| // Declarations cannot have dependencies. | ||
| // Declarations cannot have dependencies. |
There was a problem hiding this comment.
Nit: dangling whitespace?
| | Notebook | ||
| | DataPreparation; | ||
| | DataPreparation | ||
| | Test; |
There was a problem hiding this comment.
The addition of tests to the list of targets in the compiled graph and a dependency, changes in session.test backing structure is technically a breaking change.
At the very least we should bump a minor version, once we merger theses changes.
There was a problem hiding this comment.
I've since removed the dependency changes (meaning tests are in the DAG, but not connected to any other action).
Should we still do the minor version bump? Or can we keep it under patch versions until we add the new tests into the middle of the DAG?
There was a problem hiding this comment.
At the moment there are still breaking changes in Dataform Core API, specifically in Session object. Bottom line is these changes shouldn't be released under a patch, which means that we should either:
- Exercise control and block all @dataform/core releases until all breaking changes are submitted from the feature branch
- Prepare all breaking changes in a separate branch and them merge them in a single commit with a minor version bump.
At the moment we don't have any automation to enforce (1) repo-wide. I think executing (2) is easier.
There was a problem hiding this comment.
Current changes LGTM from my side, will wait for resolution in this thread to approve the PR
kolina
left a comment
kolina
left a comment
There was a problem hiding this comment.
Waiting on resolution of how to update Dataform core version with these changes
3 participants
Added new properties to Test proto, including target/canonical target and dependency targets.
Updated compilation logic to add tests as dependencies to the action being tested.