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

(FACT-2815) Added timing for cached facts #2134

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

sebastian-miclea
Copy link
Contributor

@sebastian-miclea sebastian-miclea commented Oct 12, 2020

Updated --timing option to also show time needed to read from cached. Before, cached facts were skipped.

@sebastian-miclea sebastian-miclea added the bug Something isn't working label Oct 12, 2020
@sebastian-miclea sebastian-miclea requested review from a team October 12, 2020 11:58
@puppetcla
Copy link

CLA signed by all contributors.

.and_return(cached_external_fact)
allow(File).to receive(:mtime).with(File.join(cache_dir, 'ext_file.txt')).and_return(Time.now)

cache_manager.resolve_facts(searched_facts)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add an empty line to make the separation between the Arrange, Act and Assertion parts of the unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

if Options[:timing]
time = Benchmark.measure { yield }

puts "fact `#{fact_name}`, took: #{time.format('%r')} seconds"
log = "fact '#{fact_name}', took: #{time.format('%r')} seconds"
puts "#{prefix_message} #{log}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could check if the prefix_message is not empty and a space after it. This way, you'll avoid offsetting the log message with a space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@Filipovici-Andrei
Copy link
Contributor

Filipovici-Andrei commented Oct 13, 2020

Does this PR resolve the inconsistent timing issue of cached facts?

@BogdanIrimie
Copy link
Contributor

@Filipovici-Andrei Filipovici-Andrei merged commit be66e84 into puppetlabs:main Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants