-
Notifications
You must be signed in to change notification settings - Fork 6
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
OSOE-904: Create an example of using FrontendServer and context.ExecuteJavascriptTestAsync #421
base: dev
Are you sure you want to change the base?
Conversation
…estContext where they make more sense.
// ensure the two work together. To make this happen, you need some custom logic to initialize the frontend process | ||
// after setup, with a unique port number to avoid clashes. Also the frontend may need the backend URL, whose port is | ||
// already randomized with every test. | ||
public abstract class FrontendUITestBase : UITestBase |
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.
Explain how much of this is necessary in every case, and what's specific to this sample but can be different based on the use case.
async context => | ||
{ | ||
// Don't forget that if you want to interact with the frontend manually, you can just switch the context | ||
// back to the back end and use the interactive mode extension method. |
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.
Can't you interact with the frontend in interactive mode too?
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 don't understand the question.
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 believe that this should be https://github.com/Lombiq/UI-Testing-Toolbox/pull/421/files/bdb345aa3710e59b52ce149c161b1eff6fa36e1e#r1846935116, otherwise I don't understand.
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.
Nope. The point is that you can explore the frontend in interactive mode, but you can't just call await context.SwitchToInteractiveAsync();
. You have to switch the UI test context scope back to the backend first (context.SwitchToBackend();
) or it won't work, because the await context.SwitchToInteractiveAsync();
will try to visit the page on the frontend's URL.
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 see. Can't SwitchToInteractiveAsync
detect where it's called from and change the URL accordingly? Having to remember this is exposing an implementation particularity. Or at least an overload that makes this easier to discover, something along the lines of SwitchToInteractiveModeFromFrontendWithSwitchToBackendAsync
(this is terribly long, but you get the idea).
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.
It's not just the SwitchToInteractiveAsync, but every method that uses Lombiq.Tests.UI.Shortcuts (because those controller actions are in the backend). I don't think it's viable nor worth the extra complexity.
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.
How about checking UITestContext.TenantName
for FrontendPseudoTenantName
and throw in something like an EnsureBackend()
in ShortcutsUITestContextExtensions
?
|
||
namespace Lombiq.Tests.UI.Samples.Tests; | ||
|
||
// Suppose you want to write UI tests in Javascript. Why would you want to do that? Unlikely if you are an Orchard Core |
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.
Everywhere it should be spelled "JavaScript".
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.
The file name would be better as something clearer like ExampleJavascriptTestShouldWork.mjs.
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'm referencing the same file in the sandbox, so that would be confusing. Also I don't want more JS tests than just this one so it's pointless to specify the test name in the file name.
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 see. Then JavaScriptTests.msj, rather.
if (args.length !== 3) throw new Error('Usage: node script.js driverPath startUrl tempDirectory'); | ||
const [driverPath, startUrl, tempDirectory] = args; | ||
|
||
let options = new chrome.Options() |
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.
Doesn't this mean that the JS test-related C# methods should throw an exception if the current browser is not Chrome, because this won't work?
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.
No, because they can provide their own driver config function if they want to. But I've updated the code to throw about it more explicitly in JS.
.addArguments('disable-dev-shm-usage') | ||
.addArguments('unsafely-disable-devtools-self-xss-warnings') | ||
.addArguments('disable-search-engine-choice-screen') | ||
.addArguments('--lang=en-US') |
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 should rather come from the config, like it is in WebDriverFactory
.
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 don't want to over-complicate it because every such config has to be passed into the script through command line arguments
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.
Nothing else from here needs to be passed in, however, this one I think should: this also affects the Accept-Language
header, thus the UI behavior. I.e. without settings this to the same as in C#, the C# and JS tests may see a different UI (even if they test different parts of the app, e.g. the same date for the same content item may be displayed differently).
.addArguments('disable-dev-shm-usage') | ||
.addArguments('unsafely-disable-devtools-self-xss-warnings') | ||
.addArguments('disable-search-engine-choice-screen') | ||
.addArguments('--lang=en-US') | ||
.addArguments('disable-accelerated-2d-canvas') | ||
.addArguments('disable-gpu') | ||
.addArguments('force-color-profile=sRGB') | ||
.addArguments('force-device-scale-factor=1') | ||
.addArguments('high-dpi-support=1') | ||
.addArguments('disable-smooth-scrolling') | ||
.addArguments('ignore-certificate-errors') | ||
.addArguments('--ignore-certificate-errors') | ||
.addArguments('--no-sandbox') |
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.
Shouldn't these all be the same as in WebDriverFactory
? If they should, please fix. And in any case, comment in both places if these two need to be kept in sync in some way.
…ting-toolkit.mjs if it's not Chrome.
Co-authored-by: Zoltán Lehóczky <[email protected]>
OSOE-904
Fixes #408