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

playwright: add page object for terminal #12381

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

planger
Copy link
Contributor

@planger planger commented Apr 5, 2023

What it does

With this PR, we add a Playwright page object to interact with the terminal. The terminal (xterm) uses a canvas to show the terminal contents. Thus, we use a workaround of selecting all contents and copying into the clipboard to read the output of the terminal.

Contributed on behalf of STMicroelectronics

How to test

Aside from making sure the added test runs in the CI, it'd be great to test this also on Mac and Windows locally. I confirmed that it works on Linux.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added terminal issues related to the terminal playwright issues related to playwright tests labels Apr 10, 2023
@marcdumais-work
Copy link
Contributor

@planger Sorry for the delay in reviewing this PR. Looks good so far. I have taken the liberty of enabling the Playwright test suite on all OS by adding a temporary commit on top of your contribution and pushing the result on my fork:

https://github.com/marcdumais-work/theia/actions/runs/4679291140

Windows and macOS have failed: Windows has an issue with the way THEIA_CONFIG_DIR is set in playwright package.json. On Mac, there's a failure in theia-text-editor.test.ts, but the new tests have passed. So, I think this PR does not make things worse - if we want to run these tests on the other OS, there will be a bit of work, not directly related to this contribution.

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @planger! The added infrastructure will make it easy to add more complex terminal tests later if needed.

@@ -1,5 +1,5 @@
// *****************************************************************************
// Copyright (C) 2021 logi.cals GmbH, EclipseSource and others.
// Copyright (C) 2021-2023 logi.cals GmbH, EclipseSource and others.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, it's not wrong to update the Copyright date, but as per EF guidance, we consider that git is the source of truth, and so generally do not require this be done when an existing file is modified.

xterm uses a canvas to show the terminal contents.
Thus, we use a workaround of selecting all contents and
copying into the clipboard to read the output of the terminal.

Contributed on behalf of STMicroelectronics

Change-Id: I2840425ced9dfebf24f97b4302b44d73a6c9741c
@planger
Copy link
Contributor Author

planger commented Apr 12, 2023

Thanks a lot for the review @marcdumais-work! I've created #12405 to track the issue on Windows and Mac.

@planger planger merged commit c335538 into eclipse-theia:master Apr 12, 2023
@planger planger deleted the playwright-add-terminal branch April 12, 2023 15:15
@paul-marechal paul-marechal added this to the 1.37.0 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
playwright issues related to playwright tests terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants