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

Group assertions in vhost spec #2256

Merged
merged 1 commit into from Jun 29, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Jun 29, 2022

This uses chaining to remove a lot of separate expectations. The big benefit is that it's a lot quicker to run and if one thing fails (like the whole catalog fails to compile) there is just a single error for the fragment rather than one for each test. It's also a lot shorter.

I'm submitting this as a separate PR so I'm testing on a clean slate. I do want this in #2255 because it makes moving things easier.

@david22swan
Copy link
Member

@ekohl Rubocop error unfortunatly:

Offenses:
spec/defines/vhost_spec.rb:770:13: E: Lint/Syntax: unexpected token tRPAREN
(Using Ruby 2.5 parser; configure using TargetRubyVersion parameter, under AllCops)
            )
            ^
spec/defines/vhost_spec.rb:834:13: E: Lint/Syntax: unexpected token tRPAREN
(Using Ruby 2.5 parser; configure using TargetRubyVersion parameter, under AllCops)
            )
            ^

david22swan
david22swan previously approved these changes Jun 29, 2022
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

Aside from the rubocop issue this looks good to me.

A little concerned over the way that failures that come after the first one, within the same block, may be hidden, but think the trade off is worth it.

@ekohl
Copy link
Collaborator Author

ekohl commented Jun 29, 2022

A little concerned over the way that failures that come after the first one, within the same block, may be hidden, but think the trade off is worth it.

Yes, worst case you may need multiple iterations where you fix it but IMHO it's also worth the trade off.

bastelfreak
bastelfreak previously approved these changes Jun 29, 2022
@ekohl
Copy link
Collaborator Author

ekohl commented Jun 29, 2022

I must say that I wrote this on top of #2255 and then cherry picked it. I hope I didn't miss anything in the rebase. It will also be interesting to see the timing difference between main and this PR.

This uses chaining to remove a lot of separate expectations. The big
benefit is that it's a lot quicker to run and if one thing fails (like
the whole catalog fails to compile) there is just a single error for the
fragment rather than one for each test. It's also a lot shorter.
@ekohl
Copy link
Collaborator Author

ekohl commented Jun 29, 2022

https://github.com/puppetlabs/puppetlabs-apache/runs/7110407048?check_suite_focus=true was 16 minutes vs https://github.com/puppetlabs/puppetlabs-apache/runs/7103340284?check_suite_focus=true with 20 minutes. Very unscientific, but it shaves off a few minutes, or about 20%. That's better than I was hoping for.

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

LGTM

@david22swan david22swan merged commit 5344728 into puppetlabs:main Jun 29, 2022
@ekohl ekohl deleted the simplify-vhost-specs branch June 29, 2022 11:54
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.

None yet

4 participants