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

ci(gui): Add end-to-end test scenarios where GUI plays a role #306

Merged
merged 31 commits into from
Oct 11, 2023

Conversation

CarlosNihelton
Copy link
Contributor

@CarlosNihelton CarlosNihelton commented Oct 4, 2023

Other than just starting the agent.

This starts simple, testing only the manual token input use case.

The way this is done is really interesting IMHO: we (ab)use the Flutter integration testing facility to create a self-driven compiled binary (no need to access the sources when running the application as previously done in the WSL end-to-end tests with the OOBE).

The Dart entry point file is choosen based on the definition of the MSBuild property named UP4W_END_TO_END. That could well be an environment variable, but I think it's preferrable to have it as an MSBuild property set via command line (see the changes in qa-azure.yaml introduced in the first commit of this PR).

The actual GUI's roleplay is determined by the end-to-end test case running. In the current implementation I'm using the Go test function name as a command line parameter for the Flutter process. Based on that input it decides which "test function" to run. We can only have a single one, since the GUI's role is not actually doing the assertions, but rather act as prescripted, so the Go testbed can assert on the whole system's side effects.

For reasons I'm not completely clear on, the app must be built in Debug mode for the GUI automation to behave well. Some actions were not working in Release, such as entering a String value into a TextField.

I previously explained the SetupConsole() trick implemented in the native code (windows/runner/main.cpp). It allows us to preserve the existing console behavior if the app is started via a debugger or via the Flutter tool, but changes the default behavior if the app is started via the command line (before it didn't output anything, now it does - to the parent process's console). That relies on some Flutter tool implementation details, which I think we are fine to rely on since this is only test code.

We hook into the method channel plugins.flutter.io/integration_test to be notified of the completion of test execution, so we can request the window to close.

I changed the way we start the app from the test bed, so we can pass CLI arguments and environment variable overrides. The later is useful when willing to run a specific test case with an invalid token, for example.

Closes UDENG-1021.

@CarlosNihelton
Copy link
Contributor Author

A side note: what are the servings of golangci-lint parameter --out-format=github-actions for us?
I suspect we'd have a better guidance if we remove it, because it hides the problematic line. Unless there is a missing step that allows us to turn its output into code review comments, I think we could just drop it.

@CarlosNihelton
Copy link
Contributor Author

This relies on #305, thus the base is not main.

Base automatically changed from update-subs-on-manual to main October 4, 2023 13:47
@CarlosNihelton CarlosNihelton marked this pull request as ready for review October 4, 2023 13:47
@CarlosNihelton CarlosNihelton self-assigned this Oct 4, 2023
If unset, remains the default entry point (lib/main.dart).
If the MSBuild property UP4W_END_TO_END is set to anything,
then end_to_end/end_to_end_test.dart
becomes the entry point.
Otherwise some pieces of flutter automation might misbehave.

The resulting would not be installable, though.
We need to install first the Debug version of VCLibs.
That comes with Visual Studio, so we just need to know its path.
By default, the Flutter main.cpp:

1. creates a new console if parent is a debugger.
2. or attaches to the parent console.

It does (2) assuming the parent is the flutter tool
(flutter run or flutter test - for example).
If its not, no console output would be visible.
A DeviceDesktopLoggerReader is be required to collect the std outputs.

This change adds detection for the case when the parent is a console shell
Thus, not the flutter tool.
We want to see the console output in that case.
It could well be the end-to-end test. ;)
Combined with the console setup allows compiling
a self driven app independent of the `flutter test´ tool,
that can close the window once tests finish.

This way we can prescript how the GUI must behave,
 build a package, deploy, run and
assert the side effects in the end-to-end tests.
Assertions should be minimal.
Only one test scenario must be active for the lifetime of the process.
The CLI arg[0] determines what scenario to activate.
That matches the test function name on the Go side.
There is where the assertions must exist.
There we assert the side effects on registry, distros etc.
This is merely a script (compiled to machine code, ofc)
I'll need those in the manual token case
To allow lazy consistency.
The function name is the argument the GUI expects
to figure out which scenario to activate.
It starts the GUI in the manual token input scenario
The GUI must then apply the token obtained from the environment
This asserts that previously and newly registered distros are affected
since now they have the "_Debug" suffix
in the workflow file.
Cannot rely on $env:ExtensionSdkDir
t.Name() at the start of the test function provides the function name.
wantAttached and not wantAttached
@CarlosNihelton
Copy link
Contributor Author

CarlosNihelton commented Oct 6, 2023

Pushing now updates since our review hangout 😉 (starting from 0b32a40. Prior to that is just consequence of a rebase)

The matrix is currently expressed as either Release (production)
or Debug (for end-to-end tests).

It uploads artifacts named after the current matrix mode,
So when running the tests, we don't download what we don't need.
Compatible with the changes in CI
Calling out the fact that there is a production
and an end-to-end-test-enabled version of the MSIX.
@EduardGomezEscandell
Copy link
Contributor

A side note: what are the servings of golangci-lint parameter --out-format=github-actions for us?
I suspect we'd have a better guidance if we remove it, because it hides the problematic line. Unless there is a missing step that allows us to turn its output into code review comments, I think we could just drop it.

This action seems to be broken when there are multiple workflows. It's supposed to write all failures in the workflow summary with links to the file and line.

So yeah, we should probably override this and simply print a normal output.

@CarlosNihelton
Copy link
Contributor Author

So yeah, we should probably override this and simply print a normal output.

Topic for a follow-up PR.

Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell left a comment

Choose a reason for hiding this comment

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

Small details. Very exciting progress!

.github/workflows/qa-azure.yaml Outdated Show resolved Hide resolved
.github/workflows/qa-azure.yaml Outdated Show resolved Hide resolved
.github/workflows/qa-azure.yaml Outdated Show resolved Hide resolved
doc/02.-Installation.md Outdated Show resolved Hide resolved
end-to-end/utils_test.go Outdated Show resolved Hide resolved
Finding the name of the deb artifact seems complicated.
The production MSIX is not that big.
We just need to be careful to not use it in the end-to-end tests.
If timeouts are needed, the caller must set it up accordingly.
it was inverted, causing the production to be built in debug
Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell left a comment

Choose a reason for hiding this comment

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

Almost there.

.github/workflows/qa-azure.yaml Outdated Show resolved Hide resolved
.vscode/tasks.json Outdated Show resolved Hide resolved
end-to-end/manual_token_input_test.go Show resolved Hide resolved
If we really want to ensure the distro doesn't attach,
the check must last as long as the longest possibility
in the must-attach case.
Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell left a comment

Choose a reason for hiding this comment

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

🎆

Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell left a comment

Choose a reason for hiding this comment

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

👍

CI is not happy with 15s apparently.
@CarlosNihelton
Copy link
Contributor Author

🎉

Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell left a comment

Choose a reason for hiding this comment

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

🚀

@CarlosNihelton CarlosNihelton merged commit 43482a0 into main Oct 11, 2023
32 checks passed
@CarlosNihelton CarlosNihelton deleted the e2e-gui-udeng-1021 branch October 11, 2023 13:35
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