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

More acceptance tests relevant to table permissions #101

Merged
merged 8 commits into from
Oct 2, 2024

Conversation

crisptrutski
Copy link
Collaborator

@crisptrutski crisptrutski commented Oct 1, 2024

Preparing to build confidence around a more basic table-only analysis.

Copy link
Collaborator Author

crisptrutski commented Oct 1, 2024

@crisptrutski crisptrutski marked this pull request as ready for review October 1, 2024 17:22
@crisptrutski crisptrutski requested a review from a team October 1, 2024 17:22
@crisptrutski crisptrutski self-assigned this Oct 1, 2024
@crisptrutski crisptrutski force-pushed the more-acceptance-tests branch from 02515ef to a038511 Compare October 2, 2024 05:45
(def ^:private not-implemented?
#{:basic-select})
(def global-overrides
{:basic-select :macaw.error/not-implemented})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tsmacdonald You should be able to just remove this entry to start TDD-ing the new walker.

(doseq [m test-modes]
(when-let [ts (testing (str prefix " table analysis does not throw for mode " m)
(is (ct/tables sql (opts-mode m))))]
(if (not-implemented? m)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rework to this function is essentially to get rid of this override hidden in the test, so that :error values in the expectations can do all the driving.

Comment on lines +7 to +10
{:ast-walker-1
;; TODO currently each table gets hidden by the other CTE
{:tables []
:source-columns []}}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Feels good to scope the one known false negative 😮‍💨

Base automatically changed from table-analysis-mode-acceptance-tests to master October 2, 2024 14:46
@crisptrutski crisptrutski force-pushed the more-acceptance-tests branch from 7a6e4a8 to 448554c Compare October 2, 2024 14:47
Copy link
Collaborator Author

crisptrutski commented Oct 2, 2024

Merge activity

  • Oct 2, 10:47 AM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
  • Oct 2, 11:10 AM EDT: @crisptrutski merged this pull request with Graphite.

@crisptrutski crisptrutski merged commit f69ba62 into master Oct 2, 2024
5 checks passed
@crisptrutski crisptrutski deleted the more-acceptance-tests branch October 2, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants