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: Initial E2E test framework for v2 #288

Merged
merged 38 commits into from
Mar 2, 2022

Conversation

bryphe-coder
Copy link
Contributor

@bryphe-coder bryphe-coder commented Feb 15, 2022

This brings an initial E2E test (really, an integration test - it's only running the server locally, as opposed to against a deployment - but it'd be easy to point playwright to a deployment).

Demo gif:
test2

This test exercises a minimal flow for login:

  • Run the coderd binary to start a server on 3000
  • Create an initial user as part of setup
  • Go through the login flow and verify we land on the projects page

It will be useful to have to ensure that #360 doesn't introduce a regression in the login flow

Future E2E tests that would be useful:

  • Create a project & verify it shows in the UI
  • Create a workspace and verify it shows in the UI

Some remaining work left:

  • Add test to CI (make sure playwright dependencies work correctly)
  • Call coderd directly instead of develop.sh
  • Use port 3000 instead of 8080, to exercise the non-next-dev server flow, and create an initial user.
  • Consolidate the test_results vs test-results paths
  • Hook up junit reporter output so the results can be sent to datadog
  • Add tag for datadog
  • Grab a video / gif of the test

@bryphe-coder bryphe-coder self-assigned this Feb 15, 2022
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #288 (787213c) into main (2b0f471) will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
- Coverage   67.94%   67.75%   -0.19%     
==========================================
  Files         150      150              
  Lines        8482     8482              
  Branches       72       72              
==========================================
- Hits         5763     5747      -16     
- Misses       2143     2156      +13     
- Partials      576      579       +3     
Flag Coverage Δ
unittest-go-macos-latest 66.12% <ø> (-0.25%) ⬇️
unittest-go-ubuntu-latest 67.44% <ø> (+0.13%) ⬆️
unittest-go-windows-2022 65.43% <ø> (+0.02%) ⬆️
unittest-js 66.10% <ø> (ø)
Impacted Files Coverage Δ
peer/conn.go 78.00% <0.00%> (-2.82%) ⬇️
peerbroker/proxy.go 55.97% <0.00%> (-1.26%) ⬇️
provisionerd/provisionerd.go 68.56% <0.00%> (-0.56%) ⬇️

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 2b0f471...787213c. Read the comment docs.

Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

This looks nice

@bryphe-coder
Copy link
Contributor Author

Thanks @vapurrmaid ! It's basically what we have in v1 (following the same POM structure you outlined). I'm just started with the integration-style tests first since we don't have an environment

I like the idea of having at least login verified build-over-build as we're standing stuff up

@bryphe-coder bryphe-coder marked this pull request as ready for review March 1, 2022 00:12
if: (success() || failure()) && github.actor != 'dependabot[bot]' && runner.os == 'Linux'
env:
DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY }}
DD_CATEGORY: e2e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kylecarbs - I added a new DataDog trait for test runs ("category") - intending to be one of unit | integration | e2e

Copy link
Member

Choose a reason for hiding this comment

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

This is awesome!

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Awesome change!

Comment on lines 298 to 301
os:
- ubuntu-latest
- macos-latest
- windows-2022
Copy link
Member

Choose a reason for hiding this comment

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

This is so cool! That means we can even test things like cross-os keybinds in the UI 😍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure! Ideally - we'd have a matrix of windows | macos | ubuntu x relevant browsers (edge | chrome | firefox | safari). Getting macos + safari would be a huge value add, since we saw some bugs there in v1 (ie https://github.com/coder/m/pull/11093 & https://github.com/coder/m/pull/9508)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I had to pull out Windows for now because the build is failing - some of our build scripts aren't cross-plat ready - ie, the yarn_install.sh script which fails with:

./scripts/yarn_install.sh
Windows Subsystem for Linux has no installed distributions.

Distributions can be installed by visiting the Microsoft Store:

https://github.com/coder/coder/runs/5368058980?check_suite_focus=true#step:8:6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a couple issues to break down next steps:

Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

Looks great!!!

.github/workflows/coder.yaml Show resolved Hide resolved
.gitignore Show resolved Hide resolved
site/.eslintignore Outdated Show resolved Hide resolved
site/e2e/globalSetup.ts Outdated Show resolved Hide resolved
site/e2e/tests/login.spec.ts Outdated Show resolved Hide resolved
import { SignInPage } from "../pom"

test("Login takes user to /projects", async ({ page, baseURL }) => {
await page.goto(baseURL + "/", { waitUntil: "networkidle" })
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that unpacking the baseURL works here since it didn't in v1 - good sign! I believe you can avoid doing the string concatenation here though if you define url in the POM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting that unpacking the baseURL works here since it didn't in v1 - good sign!

Yep! The v1 E2E tests use a special (external) address to point to the server under test (picked up from RUNTIME_CONFIG: https://github.com/coder/m/blob/833f23892c4802bfcdf9b39c86f415a1dc70a3b2/product/coder/e2e/configuration/runtime.ts#L18)

With these tests - using the webServer automatically populates the baseURL which is convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you can avoid doing the string concatenation here though if you define url in the POM.

Good point - we have some more utilities in the POM layer in v1, I brought them over (ie, BasePOM) here in this change: 7c568de

It's a little bit awkward - since I'm passing both the baseURL and page into the POM's - let me know if you were thinking of a different approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented @vapurrmaid 's suggestions which help improve this in: fae6b3f - I'll merge what we have so the test is in place for the NextJS -> Webpack changes, but happy to iterate on it more

Copy link
Contributor

Choose a reason for hiding this comment

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

@bryphe-coder I'm definitely in favor of this merging sooner than later for iteration, it's in a really good starting spot!

@greyscaled
Copy link
Contributor

greyscaled commented Mar 2, 2022

Thanks for this one @bryphe-coder 🎉 !

@bryphe-coder
Copy link
Contributor Author

bryphe-coder commented Mar 2, 2022

@vapurrmaid - I like the proposal, IMO it's simpler than the current implementation I have. Thanks for the suggestion!

EDIT: Incorporated it in fae6b3f

export const username = "admin"
export const password = "password"
export const organization = "acme-crop"
export const email = "admin@coder.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

@bryphe-coder thanks for implementing this! 🎉

I def didn't mean for it to block the work, but it's a nice touch 🎊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the great feedback & review @vapurrmaid !

@bryphe-coder bryphe-coder merged commit 86994da into main Mar 2, 2022
@bryphe-coder bryphe-coder deleted the bryphe/feat/initial-e2e-tests branch March 2, 2022 17:26
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.

None yet

4 participants