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

Feature/log time assert #134

Merged
merged 7 commits into from
Nov 18, 2019

Conversation

Hawk94
Copy link
Contributor

@Hawk94 Hawk94 commented Nov 17, 2019

Extend log_time decorator to handle logging from multiple different validators. Also align output from spectacles assert & spectacles sql commands:

Screenshot 2019-11-17 at 18 55 16

Screenshot 2019-11-17 at 18 55 10

@codecov
Copy link

codecov bot commented Nov 17, 2019

Codecov Report

Merging #134 into master will increase coverage by 0.28%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #134      +/-   ##
=========================================
+ Coverage   66.52%   66.8%   +0.28%     
=========================================
  Files           9       9              
  Lines         684     717      +33     
=========================================
+ Hits          455     479      +24     
- Misses        229     238       +9
Impacted Files Coverage Δ
spectacles/validators.py 57.3% <0%> (-2%) ⬇️
spectacles/runner.py 47.36% <100%> (+6.19%) ⬆️
spectacles/utils.py 91.89% <100%> (+10.64%) ⬆️
spectacles/printer.py 74% <25%> (-4.27%) ⬇️

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 09e6ff3...bb34bf7. Read the comment docs.

@DylanBaker
Copy link
Collaborator

@Hawk94 Thanks for this! It looks good.

One thing I can't decide is if the time should be how long the whole process takes, or just the validate step. There's a bit of time spent building up the LookML project, that doesn't get included if we just wrap the validate function. @joshtemple Do you have an opinion on what it should be measuring?

Copy link
Collaborator

@joshtemple joshtemple left a comment

Choose a reason for hiding this comment

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

This looks great @Hawk94! I think measuring validate is good enough. There isn't that much happening outside of that method except for instantiation of the client and branch checkout.

There's a bit of time spent building up the LookML project

This actually happens in the validate method, so we're covered here.

Copy link
Collaborator

@joshtemple joshtemple left a comment

Choose a reason for hiding this comment

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

My mistake, @DylanBaker pointed out we should decorate the validate_* methods in the Runner class, not the validate methods of the Validators. I'm assuming that's a straightforward change and helps capture more of the runtime.

@DylanBaker DylanBaker merged commit cbb17ff into spectacles-ci:master Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants