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

ShellParams: Add subshell test #922

Conversation

JohnAZoidberg
Copy link
Contributor

Separate experimental PR based on #772.
It works but I'm not sure if you agree with how it's run.


To test whether the ShellParams protocol works properly, we should try
it with different parameters, not just the default of one.

To do that we need to run the test runner again. So that it doesn't run
all of the tests again, we test whether the ShellParams protocol is
installed and whether arguments have been passed. If it's not installed
or if there are no arguments, the default tests run.
As part of ShellParams test, it launches the second instance of
test_runner.efi with parameters. That instance will only run a specific
subshell test to check if the parameters were correctly passed.

Useful to get the shell arguments for a commandline application.

Signed-off-by: Daniel Schaefer <[email protected]>
It's safer.

Signed-off-by: Daniel Schaefer <[email protected]>
To test whether the ShellParams protocol works properly, we should try
it with different parameters, not just the default of one.

To do that we need to run the test runner again. So that it doesn't run
all of the tests again, we test whether the ShellParams protocol is
installed and whether arguments have been passed. If it's not installed
or if there are no arguments, the default tests run.
As part of ShellParams test, it launches the second instance of
test_runner.efi  with parameters. That instance will only run a specific
subshell test to check if the parameters were correctly passed.

Signed-off-by: Daniel Schaefer <[email protected]>
@JohnAZoidberg
Copy link
Contributor Author

Sorry, wanted to make it a draft.

@JohnAZoidberg JohnAZoidberg mentioned this pull request Aug 21, 2023
2 tasks
@@ -45,6 +67,17 @@ fn efi_main(image: Handle, mut st: SystemTable<Boot>) -> Status {
// Test all the boot services.
let bt = st.boot_services();

// If ShellParametes protocol present and run with arguments,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// If ShellParametes protocol present and run with arguments,
// If ShellParameters protocol present and run with arguments,

@nicholasbishop
Copy link
Member

nicholasbishop commented Aug 29, 2023

To test whether the ShellParams protocol works properly, we should try it with different parameters, not just the default of one.

To do that we need to run the test runner again.

Could we instead just launch the test runner with more params in the first place? I.e. where we currently have shell.efi test_runner.efi, could we just add some extra arg1 arg2 that are unused except for this testing?

@JohnAZoidberg
Copy link
Contributor Author

Could we instead just launch the test runner with more params in the first place? I.e. where we currently have shell.efi test_runner.efi, could we just add some extra arg1 arg2 that are unused except for this testing?

Sure, just a single test case then?

@nicholasbishop
Copy link
Member

I think that would be best, yes, just to keep the test flow simpler. Unless you think there's a high chance we'd be missing bugs from not doing the extra subshell test.

@JohnAZoidberg
Copy link
Contributor Author

Ok sure, I integrated that simple test in #772

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