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

feat(testing): add e2e tests for code-server and terminal #3169

Merged
merged 6 commits into from
Apr 26, 2021

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Apr 19, 2021

This PR adds a test for the integrated terminal in code-server.

Changes

  • add CodeServer class for simplifying testing (thank you @jawnsy for the POM tip!)
  • add test for CodeServer
  • add test for terminal
  • fix expect in a couple e2e tests (I forgot .toBe(true))

Screenshots

image

69c0ddcb7541307f217a818f94992163.mp4

Checklist

  • tested locally
  • added a test
  • refactor and use temp folder for terminal test
  • use descriptive name like terminal_e2e_test.txt
  • fix CodeServer.focusTerminal() method
  • remove all TODOs for this PR
  • refactor tmpdir
  • clean up commits
  • try removing waitForTimeouts in terminal.test.ts
  • try changing esModuleInterop to false I don't want to mess with this. If someone wants to create a ticket to check out later, go for it. Otherwise, I'll add a note to the "Clean up" issue
  • address feedback

@jsjoeio jsjoeio changed the title jsjoeio/add-terminal-e2e-test feat(testing): add e2e tests for code-server and terminal Apr 19, 2021
@jsjoeio jsjoeio self-assigned this Apr 19, 2021
@jsjoeio jsjoeio added this to the v3.9.4 milestone Apr 19, 2021
@jsjoeio jsjoeio force-pushed the jsjoeio/add-terminal-e2e-test branch from ecc7d6a to 0ec7ae4 Compare April 20, 2021 23:55
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio force-pushed the jsjoeio/add-terminal-e2e-test branch 2 times, most recently from 2977bc6 to 958aa84 Compare April 22, 2021 19:17
src/node/constants.ts Show resolved Hide resolved
test/e2e/browser.test.ts Show resolved Hide resolved
test/e2e/codeServer.test.ts Show resolved Hide resolved
test/e2e/globalSetup.test.ts Show resolved Hide resolved
test/e2e/login.test.ts Show resolved Hide resolved
test/e2e/models/CodeServer.ts Show resolved Hide resolved
test/e2e/models/CodeServer.ts Show resolved Hide resolved
test/e2e/models/CodeServer.ts Outdated Show resolved Hide resolved
test/e2e/models/CodeServer.ts Show resolved Hide resolved
test/e2e/openHelpAbout.test.ts Show resolved Hide resolved
@jsjoeio jsjoeio marked this pull request as ready for review April 22, 2021 19:27
@jsjoeio jsjoeio requested a review from a team as a code owner April 22, 2021 19:27
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Apr 22, 2021

Womp. 404. Network issue. I wonder if we could automatically retry if it 404s? cc @oxy

image

@oxy
Copy link

oxy commented Apr 22, 2021

@jsjoeio sadly there's no "please re-run the same step" within an action on GitHub - and we don't have control over @actions/download-artifact's behavior either.

I think I can address this in a future PR where we split up all the things to upload to different artifacts, eg. release-package-macos instead of just everything being in release-packages - or maybe we should file a bug/support request with the actions team? looks to me kind of like a race condition

(file starts uploading from one action (eg. package-macos), and then the other action (eg. docker-arm64) tries to download it but can't because it isn't done uploading yet)

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Apr 22, 2021

address this in a future PR

Okay, sounds good!

maybe we should file a bug/support request with the actions team?

Yeah, that could be a good idea too! I'll leave that to you if you want.

@jsjoeio jsjoeio linked an issue Apr 22, 2021 that may be closed by this pull request
src/node/constants.ts Show resolved Hide resolved
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

I like the new model!

test/e2e/codeServer.test.ts Outdated Show resolved Hide resolved
test/e2e/models/CodeServer.ts Outdated Show resolved Hide resolved
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
test/e2e/terminal.test.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #3169 (7bfdd13) into main (5ad8e68) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3169      +/-   ##
==========================================
+ Coverage   46.77%   46.86%   +0.08%     
==========================================
  Files          23       23              
  Lines        1193     1195       +2     
  Branches      237      237              
==========================================
+ Hits          558      560       +2     
  Misses        451      451              
  Partials      184      184              
Impacted Files Coverage Δ
src/node/util.ts 44.56% <ø> (-0.60%) ⬇️
src/node/constants.ts 92.85% <100.00%> (+1.19%) ⬆️
src/node/socket.ts 89.65% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ad8e68...7bfdd13. Read the comment docs.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Apr 23, 2021

No coverage uploaded for pull request base (main@72ca12c)

I think this is cause I started this PR before Codecov was added. We'll see though in the next PR on code-server.

@jsjoeio jsjoeio marked this pull request as draft April 23, 2021 19:16
@jsjoeio jsjoeio force-pushed the jsjoeio/add-terminal-e2e-test branch from 8524265 to f40f468 Compare April 23, 2021 21:35
@jsjoeio jsjoeio marked this pull request as ready for review April 23, 2021 21:35
@jsjoeio jsjoeio requested a review from code-asher April 23, 2021 21:36
@jsjoeio jsjoeio force-pushed the jsjoeio/add-terminal-e2e-test branch from f40f468 to a404e59 Compare April 23, 2021 21:39
@jsjoeio jsjoeio force-pushed the jsjoeio/add-terminal-e2e-test branch from a404e59 to 7bfdd13 Compare April 23, 2021 23:40
@jsjoeio jsjoeio added the testing Anything related to testing label Apr 23, 2021
@jsjoeio jsjoeio merged commit 07d6823 into main Apr 26, 2021
@jsjoeio jsjoeio deleted the jsjoeio/add-terminal-e2e-test branch April 26, 2021 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Anything related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using Page Objects for Playwright tests
4 participants