Skip to content

Run permissions tests from Foreman core#51

Draft
ekohl wants to merge 1 commit into
theforeman:masterfrom
ekohl:run-permission-tests-from-core
Draft

Run permissions tests from Foreman core#51
ekohl wants to merge 1 commit into
theforeman:masterfrom
ekohl:run-permission-tests-from-core

Conversation

@ekohl

@ekohl ekohl commented Oct 30, 2023

Copy link
Copy Markdown
Member

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.

Comment thread lib/tasks/foreman_plugin_template_tasks.rake Outdated
@evgeni

evgeni commented Nov 28, 2023

Copy link
Copy Markdown
Member

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

@ekohl

ekohl commented Jan 12, 2024

Copy link
Copy Markdown
Member Author

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

evgeni commented Jan 12, 2024

Copy link
Copy Markdown
Member

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

ekohl commented Jan 16, 2024

Copy link
Copy Markdown
Member Author

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

evgeni commented Jan 16, 2024

Copy link
Copy Markdown
Member

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

ekohl commented Jan 22, 2024

Copy link
Copy Markdown
Member Author

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

@evgeni

evgeni commented Jan 22, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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
Copy Markdown
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I also think that this should get merged because we just stumbled across some permissions issue during plugin development and I realized only now that this permissions test got lost during the move to the unified github actions workflow.

@nadjaheitmann

Copy link
Copy Markdown

@ekohl @evgeni Can we finalize this or at least get the change for the permissions test in? I found that many plugins do not execute the access_permissions test anymore because they solely use the github action but this test is not part of the github action.

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