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

Allow model/explore selection and exclusion in assert #235

Merged
merged 11 commits into from
Jun 17, 2020

Conversation

joshtemple
Copy link
Collaborator

@joshtemple joshtemple commented Jun 16, 2020

Closes #227, closes #165. Refactors the selection logic to use regex, making it much more concise and readable. Move the --explores and --exclude arguments to the validator subparser.

The architecture of this change means it's much more difficult to validate the selectors and exclusions passed in (for example, raising an error on model_a/explore_c to communicate that explore_c doesn't exist). This isn't ideal, because it's nice to catch when people have typos in their args, for example.

@joshtemple joshtemple force-pushed the feature/assert-select branch from 763de30 to a7c0dfc Compare June 16, 2020 20:06
@joshtemple joshtemple force-pushed the feature/assert-select branch from a7c0dfc to 23741ba Compare June 16, 2020 20:10
@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #235 into master will decrease coverage by 0.09%.
The diff coverage is 95.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   89.90%   89.81%   -0.10%     
==========================================
  Files          12       13       +1     
  Lines        1060     1060              
==========================================
- Hits          953      952       -1     
- Misses        107      108       +1     
Impacted Files Coverage Δ
spectacles/runner.py 82.05% <50.00%> (ø)
spectacles/cli.py 78.70% <66.66%> (ø)
spectacles/select.py 94.44% <94.44%> (ø)
spectacles/client.py 98.64% <100.00%> (+0.51%) ⬆️
spectacles/validators.py 98.22% <100.00%> (-0.18%) ⬇️
spectacles/lookml.py 81.43% <0.00%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d65fca...a2f00d3. Read the comment docs.

@joshtemple joshtemple marked this pull request as ready for review June 16, 2020 20:57
@joshtemple joshtemple requested a review from DylanBaker as a code owner June 16, 2020 20:57
@joshtemple joshtemple force-pushed the feature/assert-select branch from 8668b0b to a2f00d3 Compare June 16, 2020 20:59
Copy link
Collaborator

@DylanBaker DylanBaker left a comment

Choose a reason for hiding this comment

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

👍 This all looks good to me. One comment in-line about parallelising the data test queries, but I think really it's probably something (if I have understood it correctly) that we should just create an issue for.

Comment on lines +115 to +123
test_results: List[Dict[str, Any]] = []
for test in selected_tests:
test_name = test["name"]
model_name = test["model_name"]
results = self.client.run_lookml_test(
self.project, model=model_name, test=test_name
)
test_results.extend(results)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there's anything we can do about this, but this will kick them off sequentially, right?

In theory, if a whole model is going to be tested, we could test it in one go I think, which would get us a decrease in run time because Looker would kick them off simultaneously. I think to do this we'd have to change the selection logic though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the most we could do with the endpoint would be to run them on a model level and then filter the results. Didn't seem worth the lift to me until there's demonstrated performance issues.

@joshtemple joshtemple merged commit f97d0a7 into master Jun 17, 2020
@joshtemple joshtemple deleted the feature/assert-select branch September 1, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants