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

Move custom variables test to the new testing structure #1601

Merged
merged 3 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions common/protos/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export interface IProtoClass<IProto, Proto> {
}

// ProtobufJS's native verify method does not check that only defined fields are present.
// Note: ProtobufJS does not do conversion of typed fields, so if a type number is present but a
// string expected, this won't throw an error.
// TODO(ekrekr): swap to Typescript protobuf library rather than having to do this; however using TS
// libraries currently available would incur a significant performance hit.
export function verifyObjectMatchesProto<Proto>(
Expand Down
116 changes: 114 additions & 2 deletions core/main_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,119 @@ suite("@dataform/core", ({ afterEach }) => {
);
});

suite("variables", () => {
test(`variables in workflow_settings.yaml must be strings`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
// fs.writeFileSync(path.join(projectDir, "workflow_settings.yaml"), `"&*19132sdS:asd:"`);
fs.writeFileSync(
path.join(projectDir, "workflow_settings.yaml"),
`
vars:
intValue: 1
strValue: "str"`
);
const coreExecutionRequest = dataform.CoreExecutionRequest.create({
compile: { compileConfig: { projectDir } }
});

expect(() => runMainInVm(coreExecutionRequest)).to.throw(
"Custom variables defined in workflow settings can only be strings."
);
});

test(`variables in dataform.json must be strings`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
path.join(projectDir, "dataform.json"),
`{"vars": { "intVar": 1, "strVar": "str" } }`
);
const coreExecutionRequest = dataform.CoreExecutionRequest.create({
compile: { compileConfig: { projectDir } }
});

expect(() => runMainInVm(coreExecutionRequest)).to.throw(
"Custom variables defined in workflow settings can only be strings."
);
});

test(`variables can be referenced in SQLX`, () => {
const projectDir = tmpDirFixture.createNewTmpDir();
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
path.join(projectDir, "workflow_settings.yaml"),
`
defaultLocation: "us"
vars:
var1: value1
var2: value2
var3: value3`
);
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.mkdirSync(path.join(projectDir, "definitions"));
fs.writeFileSync(
path.join(projectDir, "definitions/file.sqlx"),
// TODO(https://github.com/dataform-co/dataform/issues/1295): add a test and fix
// functionality for assertions overriding database.
`
config {
type: "table",
database: dataform.projectConfig.vars.var1,
description: dataform.projectConfig.vars.var2
}
select 1 AS \${dataform.projectConfig.vars.var3}`
);
const coreExecutionRequest = dataform.CoreExecutionRequest.create({
compile: { compileConfig: { projectDir, filePaths: ["definitions/file.sqlx"] } }
});

const result = runMainInVm(coreExecutionRequest);

expect(asPlainObject(result.compile.compiledGraph)).deep.equals(
asPlainObject({
dataformCoreVersion: "3.0.0",
graphErrors: {},
projectConfig: {
defaultLocation: "us",
vars: {
var1: "value1",
var2: "value2",
var3: "value3"
},
warehouse: "bigquery"
},
tables: [
{
actionDescriptor: {
description: "value2"
},
canonicalTarget: {
database: "value1",
name: "file"
},
disabled: false,
enumType: "TABLE",
fileName: "definitions/file.sqlx",
query: "\n\nselect 1 AS value3",
target: {
database: "value1",
name: "file"
},
type: "table"
}
],
targets: [
{
database: "value1",
name: "file"
}
]
})
);
});
});

// TODO(ekrekr): add a test for nested fields, once they exist.
});

Expand All @@ -170,8 +283,7 @@ suite("@dataform/core", ({ afterEach }) => {
path.join(projectDir, "definitions/actions.yaml"),
`
actions:
- fileName: definitions/notebook.ipynb
`
- fileName: definitions/notebook.ipynb`
);
// tslint:disable-next-line: tsr-detect-non-literal-fs-filename
fs.writeFileSync(
Expand Down
2 changes: 1 addition & 1 deletion core/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ export class Session {
!!this.config.vars &&
!Object.values(this.config.vars).every(value => typeof value === "string")
) {
throw new Error("Custom variables defined in dataform.json can only be strings.");
throw new Error("Custom variables defined in workflow settings can only be strings.");
}

// TODO(ekrekr): replace verify here with something that actually works.
Expand Down
25 changes: 15 additions & 10 deletions core/workflow_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,39 @@ export function readWorkflowSettings(): dataform.ProjectConfig {
if (!workflowSettingsAsJson) {
throw Error("workflow_settings.yaml is invalid");
}
verifyWorkflowSettingsAsJson(workflowSettingsAsJson);
return dataform.ProjectConfig.fromObject(workflowSettingsAsJson);
return verifyWorkflowSettingsAsJson(workflowSettingsAsJson);
}

if (dataformJson) {
verifyWorkflowSettingsAsJson(dataformJson);
return dataform.ProjectConfig.fromObject(dataformJson);
return verifyWorkflowSettingsAsJson(dataformJson);
}

throw Error("Failed to resolve workflow_settings.yaml");
}

function verifyWorkflowSettingsAsJson(workflowSettingsAsJson: object): { [key: string]: any } {
function verifyWorkflowSettingsAsJson(workflowSettingsAsJson: object): dataform.ProjectConfig {
let projectConfig = dataform.ProjectConfig.create();
try {
verifyObjectMatchesProto(dataform.ProjectConfig, workflowSettingsAsJson);
projectConfig = dataform.ProjectConfig.create(
verifyObjectMatchesProto(
dataform.ProjectConfig,
workflowSettingsAsJson as {
[key: string]: any;
}
)
);
} catch (e) {
if (e instanceof ReferenceError) {
throw ReferenceError(`Workflow settings error: ${e.message}`);
}
throw e;
}
const verifiedWorkflowSettings = workflowSettingsAsJson as { [key: string]: any };
// tslint:disable-next-line: no-string-literal
if (!verifiedWorkflowSettings["warehouse"]) {
if (!projectConfig.warehouse) {
// tslint:disable-next-line: no-string-literal
verifiedWorkflowSettings["warehouse"] = "bigquery";
projectConfig.warehouse = "bigquery";
}
return verifiedWorkflowSettings;
return projectConfig;
}

function maybeRequire(file: string): any {
Expand Down
29 changes: 0 additions & 29 deletions tests/core/core.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1079,35 +1079,6 @@ suite("@dataform/core", () => {
"A defaultLocation is required for BigQuery. This can be configured in dataform.json."
]);
});

test("variables defined in dataform.json must be strings", () => {
const sessionFail = new Session(path.dirname(__filename), {
warehouse: "bigquery",
defaultSchema: "schema",
defaultLocation: "location",
vars: {
int_var: 1,
str_var: "str"
}
} as any);

expect(() => {
sessionFail.compile();
}).to.throw("Custom variables defined in dataform.json can only be strings.");

const sessionSuccess = new Session(path.dirname(__filename), {
warehouse: "bigquery",
defaultSchema: "schema",
defaultLocation: "location",
vars: {
str_var1: "str1",
str_var2: "str2"
}
} as any);

const graph = sessionSuccess.compile();
expect(graph.graphErrors.compilationErrors).to.eql([]);
});
});

suite("compilers", () => {
Expand Down