- Notifications
You must be signed in to change notification settings - Fork928
fix: always use bash when executing web terminal tests#12755
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
54a3715
to237a5d1
CompareThere 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.
We might want to instead grab the link from that button, modify it with the query parameter, then click it.
Right now, this is technically executing two terminal sessions.
aslilac commentedMar 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I thought about that, but I'm not crazy about the feeling of reaching into the app, making changes from the outside, and then using this newly tweaked behavior as the one that we test. 😅 Feels a bit too much like we're not testing the button anymore.
|
I tried it anyway just to see, and something is getting lost in translation. // This assertion passes, everything seems fine.expect(awaitpage.getByTestId("terminal").getAttribute("href")).toBe(`/@admin/${workspaceName}.dev/terminal${commandQuery}`,);// This one does not. Somehow, the change isn't _actually_ taking effect, even though it seems to be reflected.awaitexpect(terminal).toHaveURL(`/@admin/${workspaceName}.dev/terminal${commandQuery}`,); ...so I'm just gonna go with the way it is. 😅 |
No description provided.