-
Notifications
You must be signed in to change notification settings - Fork 1.6k
User/beenachauhan/test wsladiag #13975
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
base: feature/wsl-for-apps
Are you sure you want to change the base?
User/beenachauhan/test wsladiag #13975
Conversation
test/windows/WsladiagTests.cpp
Outdated
|
|
||
| const bool noSessions = (out.find(L"No WSLA sessions found.") != std::wstring::npos); | ||
|
|
||
| const bool hasTable = (out.find(L"WSLA session") != std::wstring::npos) && (out.find(L"ID") != std::wstring::npos) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend having a utility method to verify the complete output. Something like:
ValidateOutput("list", "No WSLA sessions found.\r\n");
For instance. That would make writing future tests easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same for below tests)
test/windows/WsladiagTests.cpp
Outdated
|
|
||
| static std::wstring BuildWsladiagCmd(const std::wstring& args) | ||
| { | ||
| const auto exePath = wsl::windows::common::wslutil::GetBasePath() / L"wsladiag.exe"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetBasePath() won't work here since it will look up the current process's executable folder.
To get the installation folder where wsladiag.exe is, use : wsl::windows::common::wslutil::GetMsiPackagePath()
15256c5 to
5e41ecf
Compare
src/windows/wsladiag/wsladiag.cpp
Outdated
| catch (...) | ||
| { | ||
| const auto hr = wil::ResultFromCaughtException(); | ||
| if (hr == E_INVALIDARG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend against this, because the service API might return that, and in that case we wouldn't want to show the usage.
To show command line parsing errors, I would recommend using something similar to this:
WSL/src/windows/common/WslClient.cpp
Line 1905 in cec99af
| auto strings = wsl::windows::common::wslutil::ErrorToString(context->ReportedError().value()); |
For the user errors to work, we'd need to explicitely enable them, like this:
wsl::windows::common::EnableContextualizedErrors(false);
And then create an execution context that we initialize in main (probably with new entrypoint value for wsladiag):
wsl::windows::common::ExecutionContext context{Context::WslaDiag};
Then we can use context to actually get the error string when an exception is thrown
test/windows/WsladiagTests.cpp
Outdated
| // Initialize the tests | ||
| TEST_CLASS_SETUP(TestClassSetup) | ||
| { | ||
| VERIFY_ARE_EQUAL(LxsstuInitialize(FALSE), TRUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specific to WSL and isn't needed for WSLA tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for LxsstuUninitialize(FALSE); below
| // Validate that list command output shows either no sessions message or session table | ||
| static void ValidateListOutput(const std::wstring& out) | ||
| { | ||
| const bool noSessions = out.find(L"No WSLA sessions found.") != std::wstring::npos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I was unclear in my previous comment. What I was suggesting was to have a method that takes the expected output as a parameter and do the validation. Something like:
void ValidateWslaDiagOutput(const std::string& Cmd, const std::string& ExpectedOutput);
So we can something like:
ValidateWslaDiagOutput("list", "<expected output>");
test/windows/WsladiagTests.cpp
Outdated
| TEST_METHOD(UnknownCommand_ShowsUsage) | ||
| { | ||
| auto [out, err, code] = RunWsladiag(L"blah"); | ||
| VERIFY_ARE_NOT_EQUAL(0, code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend validating that the error code is 1 explicitly, since that's what we expect
f08388e to
87b8180
Compare
| { | ||
| wsl::windows::common::EnableContextualizedErrors(false); | ||
|
|
||
| std::optional<ExecutionContext> context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: context is always set to the same value, so it doesn't need to be an std::optional
| auto [out, err, code] = RunWsladiag(L"blah"); | ||
| VERIFY_ARE_EQUAL(1, code); | ||
|
|
||
| const std::wstring combined = out + err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd recommend validating both fds separately. For instance error messages should always be on stderr, not stdout
| static void ValidateWslaDiagOutput(const std::wstring& cmd, const std::wstring& expectedSubstring) | ||
| { | ||
| auto [out, err, code] = RunWsladiag(cmd); | ||
| const std::wstring combined = out + err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'd recommend having two parameters, one for each output so we can validate them separately.
I'll also recommend validating the entire strings, that way we'll catch if anything unexpected is printed
87b8180 to
c5a97ae
Compare
Summary of the Pull Request:
This PR adds automated test coverage for the wsladiag CLI tool under test/windows.
The new tests exercise the supported commands and common error paths to ensure argument parsing, exit codes, and output behavior remain correct.
PR Checklist
Detailed Description of the Pull Request / Additional comments
A new test file (WsladiagTests.cpp) is added to the Windows test suite. The tests invoke wsladiag.exe directly and validate:
The tests intentionally avoid interactive shell success cases and distro-dependent scenarios to keep the suite stable and fast. Assertions use substring matching rather than exact formatting to reduce flakiness.
Validation Steps Performed
Built the solution and confirmed wsladiag.exe is generated correctly