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

Run permissions tests from Foreman core #51

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

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Oct 30, 2023

Opened to showcase an alternative to theforeman/actions#16.

The idea is that plugins should always run a certain test from core. This relies on:

If both pattern and test_files are used, then the list of test files is the union of the two.

It also starts to depend on db:test:prepare since I saw that at least in foreman_tasks.

@evgeni
Copy link
Member

evgeni commented Nov 28, 2023

I wish this list (or at least its base) could come from Foreman and not be hard-coded here.

@ekohl
Copy link
Member Author

ekohl commented Jan 12, 2024

I wish this list (or at least its base) could come from Foreman and not be hard-coded here.

How do you know what is and isn't relevant from Foreman core? Today in Jenkins we run the full Foreman test suite, which makes it a lot slower but you'd test for regressions.

@ekohl ekohl force-pushed the run-permission-tests-from-core branch from a91eb4d to cf5edfa Compare January 12, 2024 14:58
@evgeni
Copy link
Member

evgeni commented Jan 12, 2024

I wish this list (or at least its base) could come from Foreman and not be hard-coded here.

How do you know what is and isn't relevant from Foreman core? Today in Jenkins we run the full Foreman test suite, which makes it a lot slower but you'd test for regressions.

I don't think there is a way to "know", but we can offer a "recommended minimum"?
So far, when I've seen plugins explicitly calling core tests, it was always the permissions test. So this "recommended minimum" would be that. And we'd write t.test_files = Foreman::Plugin.recommended_core_tests in the default Rakefile, and people can add + [whatever] if they decide they need more?

@ekohl
Copy link
Member Author

ekohl commented Jan 16, 2024

OpenSCAP has defined a specific task for it:
https://github.com/theforeman/foreman_openscap/blob/279fa50e568985b099ce8dc9ab216e82096e91e7/lib/tasks/foreman_openscap_tasks.rake#L94-L107

Now one question is: should recommended_core_tests do something smart or is it just a static list?

@evgeni
Copy link
Member

evgeni commented Jan 16, 2024

Now one question is: should recommended_core_tests do something smart or is it just a static list?

I think a static list is perfectly fine.
(Unless you already an idea how it could be "smart", but I'd not wait for that if not)

@ekohl
Copy link
Member Author

ekohl commented Jan 22, 2024

Just a crazy idea: how about some helper in Foreman itself to define the test tasks? Then allow further configuration via a block.

@evgeni
Copy link
Member

evgeni commented Jan 22, 2024

Crazy? Maybe.

IMHO one thing we should consider is time. Is this something we want to have soon or we can live without while we figure out the best implementation?

t.libs << test_dir
t.pattern = "#{test_dir}/**/*_test.rb"
Rake::TestTask.new(:foreman_plugin_template => 'db:test:prepare') do |t|
t.libs << ForemanPluginTemplate::Engine.root.join('test')
Copy link
Member

Choose a reason for hiding this comment

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

Before this, libs was ['test', File.expand_path('../../test', __dir__)], now it's only [ForemanPluginTemplate::Engine.root.join('test')], doesn't that mean the bare test one is not added anymore, which means the test helpers from Foreman are not available anymor?

Rake::TestTask.new(:foreman_plugin_template => 'db:test:prepare') do |t|
t.libs << ForemanPluginTemplate::Engine.root.join('test')
t.pattern = ForemanPluginTemplate::Engine.root.join('test', '**', '*_test.rb')
t.test_files = [Rails.root.join('test/unit/foreman/access_permissions_test.rb')]
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is the important line of this PR and we should get that merged.

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