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

Introduce "exec-runnables-recipe" resolver #6032

Merged
merged 4 commits into from
Sep 28, 2024

Conversation

clebergnu
Copy link
Contributor

This resolver is somewhat of a hybrid between the "exec-test" and the "runnables-recipe" resolvers.

It runs an executable, and attempts to read from its STDOUT content that will be treated as runnables-recipe JSON content. If that succeeds, the content will be returned as test resolutions. This is useful for executable tests or test generators that will output the tests dynamically.

Fixes: #6009

The exec-test and tap resolvers share the same code that verifies the
condition of the reference given being an executable file.

This would be enough to consolidate them into a single method, but
there's going to be a third users of the same check, so there's more
reason for that yet.

Signed-off-by: Cleber Rosa <[email protected]>
@clebergnu clebergnu added the customer:Passt Requirements/issues raised by the Passt project label Sep 18, 2024
@clebergnu clebergnu added this to the 108 - Codename TBD milestone Sep 18, 2024
@clebergnu clebergnu self-assigned this Sep 18, 2024
@clebergnu clebergnu force-pushed the resolver_exec branch 2 times, most recently from 458e53d to a882899 Compare September 18, 2024 22:26
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @clebergnu, I don't have many comments to the code itself, but I think we should discuss the overall behaviour of exec-runnables-recipe resolver.

IIUIC, with your solution we can combine exec-runnables-recipe and exec-test resolvers together, which IMO is not good behaviour, because we could end up in situations where tests are run during resolving phase. I would propose two possible solutions here.

  1. If --resolver-run-executables option is used all the other resolvers are disabled. This is easy solution, but we will lose the possibility of running multiple test types at once.
  2. --resolver-run-executables won't be a boolean, but it will be list where user can specify executables which should be directly run by exec-runnables-recipe resolver. All the other references will go through normal resolution process. I would prefer this kind of solution.

What do you think, about this problem.

avocado/plugins/resolvers.py Show resolved Hide resolved
avocado/plugins/resolvers.py Show resolved Hide resolved
@clebergnu
Copy link
Contributor Author

Hi @clebergnu, I don't have many comments to the code itself, but I think we should discuss the overall behaviour of exec-runnables-recipe resolver.

IIUIC, with your solution we can combine exec-runnables-recipe and exec-test resolvers together, which IMO is not good behaviour, because we could end up in situations where tests are run during resolving phase. I would propose two possible solutions here.

True. Although I believe the solution we have for that is really the use of different suites. Users should be aware of the behavior, and control, through the use of different suites, if executables are meant to be run during the resolver phase or not.

  1. If --resolver-run-executables option is used all the other resolvers are disabled. This is easy solution, but we will lose the possibility of running multiple test types at once.

Right.

  1. --resolver-run-executables won't be a boolean, but it will be list where user can specify executables which should be directly run by exec-runnables-recipe resolver. All the other references will go through normal resolution process. I would prefer this kind of solution.

I think this is flexible, but adds "special" options and behavior to the overall resolver approach.

What do you think, about this problem.

Like I said before, I think users should pick the references themselves that should be executed for resolutions, and the ones that should not (that will be executed as tests instead).

I'll go ahead and better document the behavior and put a note about how users can not (or should not) combine those two different types of executables in a single suite.

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @clebergnu, thank you for your update. The documentation is definitely better, but I think it still needs to be explicit. It should let users know that their tests might be executed twice with --resolver-run-executables and a way how to fix this is to run exec-resolvers and exec-tests in different suites.

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, it LGTM

@richtja
Copy link
Contributor

richtja commented Sep 27, 2024

@clebergnu, I just noticed that spell-check is failing. Can you please fix that? Thanks.

This resolver is somewhat of a hybrid between the "exec-test" and the
"runnables-recipe" resolvers.

It runs an executable, and attempts to read from its STDOUT content
that will be treated as runnables-recipe JSON content.  If that
succeeds, the content will be returned as test resolutions.  This is
useful for executable tests or test generators that will output the
tests dinamically.

Signed-off-by: Cleber Rosa <[email protected]>
This adds support for used defined arguments to be passed while
running the executables that will generate the runnables recipe JSON
content.

It gives the opportunity for either calling the executables with a
particular option that will output the runnables (instead of other
action), or tweaking the type of runnables that will be generated.

Signed-off-by: Cleber Rosa <[email protected]>
@clebergnu
Copy link
Contributor Author

@clebergnu, I just noticed that spell-check is failing. Can you please fix that? Thanks.

Spell checking is happy now! Thanks for the review!

@clebergnu clebergnu merged commit a24c5ff into avocado-framework:master Sep 28, 2024
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer:Passt Requirements/issues raised by the Passt project
Projects
Status: Done 108
Development

Successfully merging this pull request may close these issues.

Easier introspection of available tests (reading of test manifest)
2 participants