-
Notifications
You must be signed in to change notification settings - Fork 299
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
Add (frontend-based) end-to-end testing starting point #2764
Conversation
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.
Only reviewed the mage parts.
Please wait for #2751 to be merged and then base your work on top of that - it's going to be done today I believe
.mage/js.go
Outdated
@@ -229,8 +250,13 @@ func (js Js) ServeMain() error { | |||
|
|||
// Messages extracts the frontend messages via babel. | |||
func (js Js) Messages() error { | |||
isCI := os.Getenv("CI") == "true" | |||
cacheExists := pathExists("./.cache/messages") |
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 be handled via target
only, see #2751
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.
See #2764 (comment)
.mage/js_sdk.go
Outdated
@@ -58,8 +58,13 @@ func (k JsSDK) Deps() error { | |||
|
|||
// Build builds the source files and output into 'dist'. | |||
func (k JsSDK) Build() error { | |||
isCI := os.Getenv("CI") == "true" |
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 not be necessary
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.
See #2764 (comment)
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 would like to play with cypress
for a bit to implement a bunch of tests so I can review this PR more meaningfully.
Could update DEVELOPMENT.md
with instructions on how to setup and start e2e tests?
ebe9321
to
0622b5f
Compare
0622b5f
to
644f120
Compare
1840273
to
da22f9f
Compare
bcd9ee8
to
95cf206
Compare
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.
Very good start. Some suggestions additionally to my other comments:
- Lets use
data-test-id
as the test attribute.- For regular components we can use custom id like
error-notification
,login-form
and so on. I dont think that BEM is appropriate to be used as test attributes. - For fields we can simply duplicate the field name in the test attribute. Initially, I was thinking about not using additional attributes for inputs and simply use the
name
attribute, however this approach makes selecting radio buttons more verbose as all buttons have the same name. For example, for payload formatters we can usedata-test-id=types-radio_{none|javascript|…}
. For other inputs nothing else is required.
- For regular components we can use custom id like
- I would still prefer having documentation in place before merging this pr. Include:
- Quick description that we use cypress.
- We use
describe
andit
to be consistent with already existing tests using jest and avoid usingcontext
,specify
frommocha
. - How to set e2e tests locally (dashboard access?)
- Some generic guidelines you used when writing tests. We will define more strict guidelines later.
Also, @pgalic96 @mjamescompton please check this PR make sure that you understand whats happening here and run cypress locally.
cypress/integration/login.spec.js
Outdated
|
||
cy.location('pathname').should('include', '/oauth') | ||
|
||
cy.findAllByText('incorrect password or user ID').should('exist') |
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 dont think we should check for the message as it is returned by the backend. All we care about here is that the path has no /console
and the error notification is present on the screen.
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.
Also, we should group related assertions together, remove new line between assertions
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 dont think we should check for the message as it is returned by the backend. All we care about here is that the path has no
/console
and the error notification is present on the screen.
I disagree. The goal is to make the tests resemble the actual user experience as closely as possible. In this test case, a user would expect to be shown a notification that informs about the nature of the problem that lead to the submit not being successful. If we only check for the error notification, the test would also pass when there is a notification displaying irrelevant or no information. Generally speaking, the more we match realistic user expectations in our tests, the more confidence we can derive from them.
Also note that since this is an end-to-end test, it is legitimate to make assertions based on backend behavior.
Do you agree?
cypress/integration/login.spec.js
Outdated
// limitations under the License. | ||
|
||
describe('Login and OAuth authorization', () => { | ||
it('Displays an error on invalid login credentials', () => { |
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 would prefer describe
and it
descriptions to be consistent with other tests in webui
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'd like to do that in a separate PR which will also update the coding guidelines and streamlines all test names as a whole.
cypress/integration/login.spec.js
Outdated
.should('have.value', 'admin') | ||
.type('{enter}') | ||
|
||
cy.grab('overview__welcome_text', { timeout: 10000 }).should('contain.text', 'Welcome') |
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.
Is there any way to avoid custom timeouts with cy.wait
?
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 noticed that the loading time during login can vary quite a lot, especially when running in headless mode. The only way I see to account for that is increasing the timeout. Note that this does not mean that the command will wait for 10 seconds to look for the element. It means that will retry finding the elements for a maximum of 10 seconds (but usually find it sooner than that).
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.
misclicked 🤷♂️
a62794d
to
e0fbabc
Compare
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.
@rvolosatovs please review tools/mage/**
too
- name: Set up Go 1.14 | ||
uses: actions/setup-go@v1 | ||
with: | ||
go-version: '1.14' |
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.
~1.14
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.
See other workflows (in develop
)
restore-keys: | | ||
${{ runner.os }}-node- | ||
if: env.should_cache | ||
- name: Apply go modules cache |
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.
Lowercase go
if: env.should_cache | ||
- name: Make Mage | ||
run: make tools/bin/mage | ||
- name: Set up Go 1.14 |
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.
vs Go
// SQLStart starts the SQL database of the development environment. | ||
func (Dev) SQLStart() error { | ||
if mg.Verbose() { | ||
fmt.Printf("Starting SQL databases\n") |
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.
nit; Println()
without \n
// SQLMakeSnapshot stores the current cockroach data folder for later restores. | ||
func (Dev) SQLMakeSnapshot() error { | ||
if mg.Verbose() { | ||
fmt.Printf("Making DB snapshot") |
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.
Missing line ending here?
on: | ||
pull_request: | ||
paths: | ||
- '*.go' |
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.
Please make sure this is sorted disregarding the casing, just like all other workflows
name: Frontend based (cypress) | ||
runs-on: ubuntu-18.04 | ||
env: | ||
should_cache: true # Might be useful for debugging. |
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 might be useful, but we're not using it now, so remove
id: yarn-cache-dir-path | ||
run: echo "::set-output name=dir::$(npx yarn cache dir)" | ||
if: env.should_cache | ||
- name: Apply yarn cache |
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.
Is there a reason not just copy-paste the same thing we have in all other workflows?
- name: Initialize Yarn module cache
uses: actions/cache@v2
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-
restore-keys: | | ||
${{ runner.os }}-yarn-cache- | ||
if: env.should_cache | ||
- name: Apply npm cache |
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.
Since we're using yarn
, npm
cache should not be necessary.
In any case if you do this, you should do this in all other workflows as well that use JS caching.
restore-keys: | | ||
${{ runner.os }}-node- | ||
if: env.should_cache | ||
- name: Apply go modules cache |
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.
Again, don't reinvent the wheel - just copy-paste what we already have.
isCI := os.Getenv("CI") == "true" | ||
dumpExists := pathExists(filepath.Join(".cache", "sqldump.sql")) | ||
ok, err := target.Dir( | ||
filepath.Join(".cache", "/sqldump.sql"), |
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.
filepath.Join(".cache", "/sqldump.sql"), | |
filepath.Join(".cache", "sqldump.sql"), |
|
||
func (EndToEnd) prepareDB() error { | ||
isCI := os.Getenv("CI") == "true" | ||
dumpExists := pathExists(filepath.Join(".cache", "sqldump.sql")) |
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.
redundant due to target.Dir
call
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.
Please acknowledge.
} | ||
|
||
func (EndToEnd) runCypress(command string, args ...string) error { | ||
return runYarnCommand("cypress", append([]string{command, fmt.Sprintf("--config-file=%s", filepath.Join("config", "cypress.json")), "--config", fmt.Sprintf("baseUrl=%s", getTestURL())}, args...)...) |
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.
IDEM
if nodeEnv == "development" { | ||
port = "8080" | ||
} | ||
return "http://localhost:" + port |
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.
Use fmt.Sprintf
|
||
var ( | ||
testDatabaseName = "ttn_lorawan_test" | ||
databaseURI = "postgresql://root@localhost:26257/" + testDatabaseName + "?sslmode=disable" |
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.
use fmt.Sprintf
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.
Please revise endToEnd
mage targets, there are quite some inconsistencies most probably because of rebasing
To run end-to-end tests, observe the following mage target: | ||
|
||
```bash | ||
$ ./mage endToEnd:run |
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.
$ ./mage endToEnd:run | |
$ tools/bin/mage endToEnd:run |
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.
$ ./tools/bin/mage -v endToEnd:run
Running target: EndToEnd:Run
Running end-to-end tests
Running dependency: go.thethings.network/lorawan-stack/tools/mage.EndToEnd.RunFrontend
Running end-to-end frontend based tests
Running dependency: go.thethings.network/lorawan-stack/tools/mage.EndToEnd.Cypress
Running dependency: go.thethings.network/lorawan-stack/tools/mage.EndToEnd.WaitUntilReady
Waiting for the stack to be ready...
Running dependency: go.thethings.network/lorawan-stack/tools/mage.Js.Deps
Error: Could not connect to server
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.
Additionally, you need to run the stack manually, this is not part of the target.
From the OP:
To run the tests you need to do the following;
mage endToEnd:prepare
only once initially, to build the stack and initialize the DB- Either:
- Run the stack with
export TTN_LW_IS_DATABASE_URI="postgresql://root@localhost:26257/ttn_lorawan_test?sslmode=disable"
- Run the mage target
mage endToEnd:runTestStack
, which will do the same, but supress the output- If you run in development mode, you also need to run
mage js:serve
mage endToEnd:run
True though that this info lacks in DEVELOPMENT.md
It is possible to run the tests in interactive mode, which is a helpful tool during development. To run the tests in interactive mode, you need to run the following command to prepare the environment: | ||
|
||
```bash | ||
$ ./mage endToEnd:prepare |
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.
$ ./mage endToEnd:prepare | |
$ tools/bin/mage endToEnd:prepare |
This will build assets and set up the testing database. Then, you can run the stack in testing configuration via | ||
|
||
```bash | ||
$ ./mage endToEnd:startTestStack |
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.
$ ./mage endToEnd:startTestStack | |
$ tools/bin/mage endToEnd:startTestStack |
Now, you can run the test in interactive mode via | ||
|
||
```bash | ||
$ ./mage endToEnd:runOpen |
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.
$ ./mage endToEnd:runOpen | |
$ tools/bin/mage endToEnd:runOpen |
|
||
##### Guding Principle | ||
|
||
We follow the [guiding principle of Kent C. Dodds](https://twitter.com/kentcdodds/status/977018512689455106) ([@kentcdodds](https://twitter.com/kentcdodds)) for writing useful tests: |
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.
Do we really need a link to a twitter post?
// DBDump performs a database dump to the .cache folder. | ||
func (EndToEnd) DBDump() error { | ||
if mg.Verbose() { | ||
fmt.Printf("Execute database dump\n") |
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.
fmt.Printf("Execute database dump\n") | |
fmt.Println("Execute database dump") |
func (EndToEnd) DBRestore() error { | ||
mg.Deps(Js.Deps, Dev.DBStart) | ||
if mg.Verbose() { | ||
fmt.Printf("Restore database from dump") |
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.
fmt.Printf("Restore database from dump") | |
fmt.Println("Restore database from dump") |
return targetError(err) | ||
} | ||
if !ok || (isCI && dumpExists) { | ||
mg.SerialDeps(EndToEnd.DBRestore) |
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.
mg.SerialDeps(EndToEnd.DBRestore) | |
mg.Deps(EndToEnd.DBRestore) |
if mg.Verbose() { | ||
fmt.Println("Running end-to-end frontend based tests") | ||
} | ||
mg.Deps(EndToEnd.Cypress) |
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.
Why not moving Cypress
here instead of using mg.Deps
for chaining target calls?
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.
cy.login() | ||
cy.visit('/console') | ||
cy.get('header').within(() => { | ||
cy.findByText('admin').click() |
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.
For me it reads like: there is the profile-dropdown
entry in the header
and it should display the users id
cy.findByText('admin').click() | |
cy.findByTestId('profile-dropdown') | |
.should('contain', 'admin') | |
.click() |
this should possible as should
returns the same subject it was given
Closing this since I don't think I will be able to bring this into an agreeable format within a reasonable amount of time. I can't spend more time on this currently and I think there are other people better suited to build this more quickly and thoroughly. If anyone wants to pick this up, feel free to use the branch as foundation/inspiration. |
Not so fast. The scope of these tests, end-to-end, mean that there are a lot of concerns to be taken into account. It's perfectly fine to agree to disagree on things – I think all contributors commit to the importance of having end-to-end tests to catch regressions, and that those concerns outweigh concerns about implementation details. |
I think it's very important to have end-to-end tests, however we should not make our own lives harder by introducing hacks and workarounds in our build process to allow for that - those come with a pretty high maintenance cost and later we end up with the same issue as was with the JS-related mage targets, where everything depends on everything, things break with no apparent reason and issues are very hard to debug/track and fix. |
Will reopen a new PR |
@bafonins @kschiffer please delete this branch when no longer necessary |
Summary
References #2605
This PR adds an initial state of (frontend-based) end-to-end testing to our codebase.
Changes
endToEnd:*
mage targets to setup and run end-to-end tests in various configurationsEnd to End Tests
GitHub ActionTesting
Manual testing, running and verifying GitHub Actions on my private fork of TTS
Regressions
This might break affected
dev/js/jsSdk
mage targets.Notes for Reviewers
Cypress
So as discussed in #2605, this uses Cypress as testing engine. I've included a couple of initial tests as well as some utility function that will help us logging in quickly, seeding the database (between runs) and grabbing DOM elements based on the
test-id
attribute.To run the tests you need to do the following;
mage endToEnd:prepare
only once initially, to build the stack and initialize the DBexport TTN_LW_IS_DATABASE_URI="postgresql://root@localhost:26257/ttn_lorawan_test?sslmode=disable"
mage endToEnd:runTestStack
, which will do the same, but supress the outputmage js:serve
mage endToEnd:run
Cypress offers an interactive mode, in which you can watch and debug the tests as you write them. You can run this via
mage endToEnd:cypressOpen
The tests will run in a
ttn-lorawan-test
database so that there will be no clashes with the development database. Note thatmage dev:DBErase
will also delete this database.@bafonins @pgalic96 @mjamescompton (and anyone who wants to write end-to-end tests for that matter), I recommend to read through the cypress docs since we will be making extensive use of these end-to-end tests once this is merged. The docs are very detailed and should give a good grasp of how to write cypress tests.
I also recommend taking note of the best practices and especially the section about "selecting elements" since we need to avoid selecting elements by arbitrary attributes that might not be persistent. Especially selecting by class is to be avoided, since the rendered classnames include arbitrary hashes. I've also included
testing-library
, which should help us selecting elements in a way that resembles actual use of the frontend, making the tests more robust and avoiding unnecessary refactors for unrelated code changes as much as possible.I suggest that we keep track of our decisions of how to write cypress tests in
DEVELOPMENT.md
.Happy to hear your thoughts, suggestions, etc. Let's try to get a robust setup going.
Github Action / CI
You can have a look at the actions here.
I've spent quite some time to get a good caching mechanism going, which can bring the runtime down to ~4mins in optimal cases. For usual JS changes, it should be around ~6min, which I think is reasonable enough. I've also disabled the caching for pushes to
develop
ormaster
to ensure that there are no flakes going through from the caching mechanism.The action is set up to run for pull requests as well as pushes to
master
ordevelop
.For failed runs, the Github Action will include a screenshot of the failed run as well as the video recordings of all tests. This data can be obtained from the "artifacts" section of the action.
@rvolosatovs there will be quite some conflicts with your recent mage refactors, I guess. I'll try to resolve these once your branches are merged.
Checklist
README.md
. The target branch is set tomaster
if the changes are fully compatible with existing API, database, configuration and CLI.CHANGELOG.md
.CONTRIBUTING.md
, there are no fixup commits left.