Spike: Running Mounted LB3 App Tests #5251

nabdelgadir · 2020-04-26T07:12:50Z

Related to #5004.

Proposal for running acceptance and integration tests. Details can be found in SPIKE.md.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

examples/lb3-application/lb3app/test/acceptance.js

bajtos · 2020-04-27T13:53:15Z

Thank you @nabdelgadir for looking into ways how to run tests of a LB3 app from inside LB4! I'll try to review the proposal tomorrow 🤞

bajtos

I like the proposed design 👍

Let's polish the details now and make sure the migration requires as little changes in the original test code as possible. A typical application can have dozens of test files with hundreds of tests, every additional step would create a lot of work for our users.

SPIKE.md

bajtos · 2020-04-28T13:30:35Z

examples/lb3-application/lb3app/test/acceptance.js

+        user,
+      ) {
+        user.email.should.be.equal('new@email.com');
+        json('post', '/api/users/login')


Please find a way how to allow this test to call User.login directly via JavaScript API, so that users don't have to rework all their tests calling model API from JavaScript to make HTTP requests instead.

Is this perhaps trying to workaround the problem that User has unknown type? (See the discussion below.)

IIUC...I think Nora tests the endpoint instead of the API here because it's acceptance test :p The APIs are tested in the integration test file.

I realize now this file is a bit confusing because I was trying to mimic the original acceptance tests which makes this now acceptance + integration but I think I'll put the auth tests in their own file.

Here is my point of view: this acceptance test has two parts. In the first part, we want to setup data (e.g. User instance, a valid AccessToken, etc.). In the second part, we test the actual scenario.

I am arguing that for test setup, we should use the most performant solution. This part is not testing anything, it's just preparing data to enable the scenario being tested. That's why I prefer to call model methods directly via JS API. HTTP calls introduce non-negligible slow down.

I am arguing that for test setup, we should use the most performant solution. This part is not testing anything, it's just preparing data to enable the scenario being tested. That's why I prefer to call model methods directly via JS API. HTTP calls introduce non-negligible slow down.

Oh I see, thanks for explanation 👍

examples/lb3-application/lb3app/test/acceptance.js

packages/booter-lb3app/src/lb3app.booter.ts

jannyHou

👏 Very useful guide and good job of the spike! Mostly LGTM, a few suggestions.

jannyHou · 2020-04-28T23:55:16Z

examples/lb3-application/package.json

    "eslint:fix": "npm run eslint -- --fix",
    "pretest": "npm run build",
-    "test": "lb-mocha \"dist/__tests__/**/*.js\"",
+    "test": "lb-mocha \"dist/__tests__/**/*.js\" \"lb3app/test/*.js\"",


jannyHou · 2020-04-29T00:06:35Z

examples/lb3-application/lb3app/test/acceptance.js

+
+let app;
+
+function json(verb, url) {


nitpick: the function name is a bit confusing, maybe call it as setContentType?
And why the test has to set the content type and Accept as application/json then send the payload? Is it also part of the migration guide?
(hmm, "migration" may not be the accurate word haha)

I thought this was traditional lb3 naming because it's what I've seen in lb3 examples (like here and here) 🙈

And why the test has to set the content type and Accept as application/json then send the payload? Is it also part of the migration guide?

Since we want users to change as little code as possible, I maintained the function as is, but I agree we can rename the function name 😄

Renamed to request to match what the function does; what do you think?

I see, I didn't realize it's from LB3...then I am ok to keep the name as it is. Since it's migration.

examples/lb3-application/lb3app/test/acceptance.js

SPIKE.md

bajtos

The new version looks pretty good! I have few minor comments to consider, see below.

I don't want to block progress, feel free to land the changes after getting approval from @jannyHou, no need to wait for me.

SPIKE.md

examples/lb3-application/lb3app/test/acceptance.js

examples/lb3-application/lb3app/test/authentication.js

packages/booter-lb3app/src/__tests__/acceptance/booter-lb3app.acceptance.ts

jannyHou

👍 LGTM

nabdelgadir · 2020-04-30T20:33:56Z

Created followup stories:

Binding LB3 models to LB4 context Documentation for Migrating LB3 Tests Mounted on an LB4 App #5298

These two should be completed after the first one:

Add example tests for LB3 app Add Example LB3 Tests to lb3-application #5300
Documentation Documentation for Migrating LB3 Tests Mounted on an LB4 App #5298

Closing the spike as done.

nabdelgadir self-assigned this Apr 26, 2020

nabdelgadir commented Apr 26, 2020

View reviewed changes

examples/lb3-application/lb3app/test/acceptance.js Outdated Show resolved Hide resolved

nabdelgadir marked this pull request as ready for review April 26, 2020 07:42

nabdelgadir requested review from bajtos, deepakrkris and raymondfeng as code owners April 26, 2020 07:42

nabdelgadir added feature Migration spike labels Apr 26, 2020

nabdelgadir force-pushed the spike/lb3test branch 2 times, most recently from cd2fec6 to ff8902f Compare April 27, 2020 18:03

bajtos suggested changes Apr 28, 2020

View reviewed changes

nabdelgadir force-pushed the spike/lb3test branch from ff8902f to 5744661 Compare April 28, 2020 15:55

nabdelgadir requested a review from bajtos April 28, 2020 15:55

jannyHou reviewed Apr 29, 2020

View reviewed changes

SPIKE.md Outdated Show resolved Hide resolved

jannyHou reviewed Apr 29, 2020

View reviewed changes

SPIKE.md Show resolved Hide resolved

nabdelgadir mentioned this pull request Apr 29, 2020

fix(booter-lb3app): rename ds binding key #5287

Merged

7 tasks

nabdelgadir force-pushed the spike/lb3test branch from 5744661 to d05de35 Compare April 29, 2020 17:02

bajtos approved these changes Apr 30, 2020

View reviewed changes

nabdelgadir added 9 commits April 30, 2020 14:18

feat(example-lb3-application): add lb3 tests to mocha

57187a9

test(example-lb3-application): acceptance tests

d6329b8

feat(booter-lb3app): bind lb3 models to context

458e0c2

test(example-lb3-application): integration tests

31d552c

style(example-lb3-application): fix linter errors

176651d

fix: add links to spike

230d4c4

fixup! fix assertions

c6541a8

fixup! fix failing test

01e6d3b

fixup! apply feedback

0776ac2

fixup! apply janny feedback

81fc3f2

nabdelgadir force-pushed the spike/lb3test branch 2 times, most recently from 0eb9c17 to 59621e7 Compare April 30, 2020 19:17

fixup! more feedback

a823ca8

nabdelgadir force-pushed the spike/lb3test branch from 59621e7 to a823ca8 Compare April 30, 2020 19:24

jannyHou approved these changes Apr 30, 2020

View reviewed changes

This was referenced Apr 30, 2020

Documentation for Migrating LB3 Tests Mounted on an LB4 App #5298

Closed

Bind Mounted LB3 Models to LB4 Context #5299

Closed

Add Example LB3 Tests to lb3-application #5300

Closed

nabdelgadir closed this Apr 30, 2020

nabdelgadir mentioned this pull request Apr 30, 2020

Spike: how to run LB3 tests from LB4 when LB3 is mounted on the LB4 app #5004

Closed

6 tasks

Spike: Running Mounted LB3 App Tests #5251

Spike: Running Mounted LB3 App Tests #5251

Uh oh!

Conversation

nabdelgadir commented Apr 26, 2020

Checklist

Uh oh!

Uh oh!

bajtos commented Apr 27, 2020

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

nabdelgadir commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jannyHou Apr 29, 2020 •

edited

Loading