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

run tests in CI #274

Merged
merged 1 commit into from
May 17, 2022
Merged

run tests in CI #274

merged 1 commit into from
May 17, 2022

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented May 13, 2022

Description

This PR runs automated tests in PRs via a github workflow

Checklist

  • I have signed the CLA
  • I have generated docs locally, and this change appears to resolve the stated issue
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label May 13, 2022
Copy link
Contributor

@leahwicz leahwicz left a comment

Choose a reason for hiding this comment

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

LGTM!

on:
pull_request:
branches: [ main ]

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would scope down the GitHub token to only have limited read permissions

permissions:
  contents: read

@@ -442,19 +442,19 @@ test("Test getting nodes by test type", () => {
expect(
matchByTestType('data')
).toStrictEqual(
[7]
[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leahwicz could you (or someone from the Core team) confirm that the changes to these tests are correct? I think that these test were invalid and, if I'm reading this correctly, the node with id=7 is a generic/schema, not a singular/data test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emmyoop could you be a second pair of eyes here please?

Copy link
Member

Choose a reason for hiding this comment

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

@drewbanin agreed that the test type seems incorrect. Good catch. While in there, you should add a node (id=8) for a singular/data test to test against on lines 445/451 to make the test a bit more complete.

@drewbanin drewbanin requested a review from emmyoop May 17, 2022 14:03
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Looks good. Always nice to get a touch more testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants