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

Speedup tests: execute OS independent code once #486

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

deric
Copy link
Contributor

@deric deric commented Aug 27, 2024

This PR might improve speed of rspec tests, ~2k test cases removed.

OS independent tests will be executed only once instead of running for each supported platform.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I have mixed feelings about this. On the one hand, we're certainly testing way too many combinations that really don't need to be tested. On the other hand, it can also be very easy to miss it when we do become more fact dependent.

Another option is to introduce an alternative to on_supported_os. Something like on_reduced_supported_os that could be smarter and filter out some combinations: if RHEL is supported and also all compatible versions then it's sufficient to only test RHEL. That's something we can also reuse in many other modules to cut down on CI testing.

Copy link
Member

Choose a reason for hiding this comment

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

At least this file isn't entirely fact independent since it uses systemd_internal_services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but $fact['systemd_internal_services'] isn't defined in any OS family hiera data/. During tests it's currently using the same value from default_module_facts.yml for all test cases.

selinux_ignore_defaults: false
)
}
_, facts = on_supported_os.first
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can introduce a helper method default_os_facts. I also wouldn't mind knowing which OS is being tested on.

@deric
Copy link
Contributor Author

deric commented Aug 29, 2024

On the other hand, it can also be very easy to miss it when we do become more fact dependent.

When you add new feature, you should write tests to cover that feature. By executing the same tests many times with different OS facts, you won't improve quality of tests (when the code doesn't depend on those facts).

Another option is to introduce an alternative to on_supported_os. Something like on_reduced_supported_os that could be smarter and filter out some combinations.

Having a method like on_supported_family where for each OS family it would pick e.g. the latest supported OS would be nice. It

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.

2 participants