[Frontend] unit tests for node server #2745

eterna2 · 2019-12-17T11:30:04Z

WIP: Adding basic unit tests for frontend node server.

PS: I noted some errors inside because seems like my new deployment cannot access the pod logs from my S3 buckets. Writing some unit tests to narrow down where are the bugs (could be my deployment issue also - cuz it worked last time, stop working after I upgrade the version).

See #2407
cc: @Bobgy

This change is

k8s-ci-robot · 2019-12-17T11:30:19Z

Hi @eterna2. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Bobgy

I'll look into details later, but your efforts are really appreciated.
Thanks a lot!

/ok-to-test

frontend/server/aws-helper-test.ts

eterna2 · 2019-12-20T11:49:22Z

I need break up the big server.ts file so that unit test can be done more easily.

Bobgy · 2019-12-21T11:51:07Z

@eterna2 Thanks, that sounds reasonable.

Note that there are some ongoing work on https://github.com/kubeflow/pipelines/pull/2690/files which also touches node server. Beware of that and I recommend don't do too much refactoring that may conflict with it.

eterna2 · 2019-12-23T10:23:55Z

Summary of changes:

unit tests for aws-helper and minio-helper (I found my bug, aws iam credential should work even when the session token expires now)
broke up setup.ts into
- app.ts (registering all the middleware),
- handlers/*.ts (defining the individual middleware), and
- configs.ts (functions to parse argv and env vars into a config singleton)
e2e unit test for the express app:
- test for gcs, s3, minio, and http/https artifacts
- test for healthz
- test for index.html

Impacts on other PRs

minor impact for Support choosing tensorboard version from UI #2690, but not a difficult change, just move server.ts changes to handlers/*.ts, no impact for k8s-helper.ts as it is not changed.

TODOs (in future PR?)

refractor k8s-helper (current state makes it impossible to do any unit test or mocking)
- BLOCKS:
  - unit test for workflow-helper
  - e2e unit test for express app: for k8s-related routes (can't test, cuz can't mock k8s api)
probably shld add a real e2e test for the proxies in the integration test

eterna2 · 2019-12-23T11:56:16Z

I updated @types/node to fix bug introduced by the recent updates: microsoft/TypeScript#32333

And updated @kubernetes/node-client to maintain compatibility with the updated @types/node.

This shld fix the docker build errors.

Bobgy

Looks good overall.
Left some minor comments.

frontend/.gitignore

frontend/server/handlers/healthz.ts

frontend/server/app.ts

Bobgy · 2019-12-24T07:30:45Z

frontend/server/app.ts

+  app: Application;
+  httpServer?: Server;
+
+  constructor(public readonly options: IConfigs) {


I'm not familiar with node common patterns, is using constructor with heavy initialization logic common?
I'd personally prefer using a pure function, e.g. initUIServer(options) to create an instance of UIServer. Extra benefit is that, the creator function can be async too (I guess it's arguable whether we need this or not).

This is just discussion. What do you think?

classes are just syntactic sugars in js. I think there are no right common patterns (as in it is fine either way).

The main reason I did this is so that I write less code in my unit test. It is easier for me to do the setup and teardown if I only need to track a single object. But I probably can do the same with httpServer thou.

But I think this is perfectly fine too.

const app = initUIServer(options); const server = app.listen(options.port, ()=>console.info(`listening to port:${port}.`); server.close();

Bobgy · 2019-12-24T07:32:54Z

/test kubeflow-pipeline-sample-test

…n be written more easily.

…e version

…t a string

eterna2 · 2019-12-26T09:54:01Z

Renamed all the interface (removed I)
Rebased to include the recent PR with tensorboard versioning
Added comments and fixed some lint/format errors

Bobgy · 2019-12-27T01:31:22Z

kubeflow-pipeline-sample-test test is flaky, we can retry later

Bobgy

This is great contribution! Having these tests in place will raise confidence and productivity of future development of this node server. Thanks for filling in the missing piece!

I finished the thorough review and left some (mostly minor) comments. Feel free to argue back as you see reasonable.

frontend/server/utils.ts

frontend/server/handlers/tslint.json

frontend/server/handlers/tensorboard.ts

frontend/server/handlers/index-html.ts

frontend/server/handlers/pod-logs.ts

frontend/server/handlers/artifacts.ts

frontend/server/configs.ts

frontend/server/app.ts

frontend/server/app.test.ts

Bobgy · 2019-12-27T03:59:21Z

@eterna2 Here's how to integrate these tests in our CI.

This is the entrypoint for kubeflow-pipeline-frontend-test:

pipelines/frontend/package.json

Line 60 in 488805c

    
           "test:ci:prow": "npm set unsafe-perm true && npm ci && npm run test:ci && ./scripts/report-coveralls.sh",

You can add an entry for calling server unit tests. It's fine doing it without coverage report. I can add coverage data later.

eterna2 · 2020-01-02T11:54:54Z

Resolved all ur comments. Rewrote pod logs handler to be more functional. And added corresponding unit tests.

Also updated test:prow.

Bobgy · 2020-01-03T01:14:48Z

Awesome, I will review today

Bobgy

Thanks for the contribution again!
/lgtm
/approve

Bobgy · 2020-01-03T07:46:08Z

frontend/server/minio-helper.test.ts

-    (awsInstanceProfileCredentials.ok as jest.Mock).mockClear();
+    (MinioClient as any).mockReset();
+    (awsInstanceProfileCredentials.getCredentials as jest.Mock).mockReset();
+    (awsInstanceProfileCredentials.ok as jest.Mock).mockReset();


Can you use https://jestjs.io/docs/en/jest-object#jestresetallmocks instead?

ok fixed all the tests.

k8s-ci-robot · 2020-01-03T08:07:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Bobgy · 2020-01-03T08:08:05Z

/hold
It seems e2e tests are failing. let me retry first
kubeflow-pipeline-e2e-test

Bobgy · 2020-01-03T08:08:13Z

/test kubeflow-pipeline-e2e-test

Bobgy · 2020-01-06T03:55:15Z

/hold cancel
Thanks! Looks great

Bobgy · 2020-01-06T04:42:06Z

/lgtm

* Add unit tests for aws-helper and minio-helper. * Break up server.ts into app.ts and handlers/*.ts so that unit test can be written more easily. * update @types/node because of microsoft/TypeScript#32333 * upgrade kubernete-client version to be compatible with new @types/node version * fix bug: AWSInstanceProfileCredentials.profile() returns a promise not a string * remove I prefix from interfaces. minor fixes on lint errors. * fix format errors * Added more unit tests, and minor fixes. * Fix comment: GKS should be GKE * use jest.resetAllMocks instead of reset each mock individually * fix format for helper test

k8s-ci-robot added the do-not-merge/work-in-progress label Dec 17, 2019

k8s-ci-robot requested review from Bobgy and IronPan December 17, 2019 11:30

k8s-ci-robot added size/XXL needs-ok-to-test labels Dec 17, 2019

Bobgy reviewed Dec 17, 2019

View reviewed changes

frontend/server/aws-helper-test.ts Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Dec 17, 2019

Bobgy mentioned this pull request Dec 17, 2019

[Frontend] Server Testing - test infra for pipeline ui node server #2215

Closed

4 tasks

eterna2 force-pushed the eterna2/frontend-server-test branch from 359cb17 to c28391f Compare December 18, 2019 04:30

eterna2 force-pushed the eterna2/frontend-server-test branch from 383160b to 46c31f4 Compare December 23, 2019 10:11

eterna2 changed the title ~~[WIP] [Frontend] unit tests for node server~~ [Frontend] unit tests for node server Dec 23, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Dec 23, 2019

Bobgy reviewed Dec 24, 2019

View reviewed changes

eterna2 added 6 commits December 26, 2019 17:03

Add unit tests for aws-helper and minio-helper.

da00f00

Break up server.ts into app.ts and handlers/*.ts so that unit test ca…

f9ddda7

…n be written more easily.

update @types/node because of microsoft/TypeScript#32333

70b1cd1

upgrade kubernete-client version to be compatible with new @types/nod…

a4957eb

…e version

fix bug: AWSInstanceProfileCredentials.profile() returns a promise no…

2ce384f

…t a string

remove I prefix from interfaces. minor fixes on lint errors.

82fef67

eterna2 force-pushed the eterna2/frontend-server-test branch from aebd6ff to 82fef67 Compare December 26, 2019 09:21

fix format errors

ef8fd7b

Bobgy reviewed Dec 27, 2019

View reviewed changes

Added more unit tests, and minor fixes.

f3261ee

Fix comment: GKS should be GKE

9578c4d

Bobgy reviewed Jan 3, 2020

View reviewed changes

k8s-ci-robot assigned Bobgy Jan 3, 2020

k8s-ci-robot added the lgtm label Jan 3, 2020

k8s-ci-robot added the approved label Jan 3, 2020

k8s-ci-robot added the do-not-merge/hold label Jan 3, 2020

use jest.resetAllMocks instead of reset each mock individually

8edbdff

k8s-ci-robot removed the lgtm label Jan 5, 2020

fix format for helper test

6683b07

k8s-ci-robot removed the do-not-merge/hold label Jan 6, 2020

k8s-ci-robot added the lgtm label Jan 6, 2020

k8s-ci-robot merged commit 2ec2fd2 into kubeflow:master Jan 6, 2020

Bobgy mentioned this pull request Jan 7, 2020

[UI] fix artifact handler query parameter key #2809

Merged

Bobgy added the area/frontend label Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frontend] unit tests for node server #2745

[Frontend] unit tests for node server #2745

eterna2 commented Dec 17, 2019 •

edited by jlewi

Loading

k8s-ci-robot commented Dec 17, 2019

Bobgy left a comment

eterna2 commented Dec 20, 2019

Bobgy commented Dec 21, 2019

eterna2 commented Dec 23, 2019 •

edited

Loading

eterna2 commented Dec 23, 2019 •

edited

Loading

Bobgy left a comment

Bobgy Dec 24, 2019

eterna2 Dec 26, 2019 •

edited

Loading

Bobgy commented Dec 24, 2019

eterna2 commented Dec 26, 2019

Bobgy commented Dec 27, 2019

Bobgy left a comment

Bobgy commented Dec 27, 2019

eterna2 commented Jan 2, 2020

Bobgy commented Jan 3, 2020

Bobgy left a comment

Bobgy Jan 3, 2020

eterna2 Jan 5, 2020

k8s-ci-robot commented Jan 3, 2020

Bobgy commented Jan 3, 2020

Bobgy commented Jan 3, 2020

Bobgy commented Jan 6, 2020

Bobgy commented Jan 6, 2020

[Frontend] unit tests for node server #2745

[Frontend] unit tests for node server #2745

Conversation

eterna2 commented Dec 17, 2019 • edited by jlewi Loading

k8s-ci-robot commented Dec 17, 2019

Bobgy left a comment

Choose a reason for hiding this comment

eterna2 commented Dec 20, 2019

Bobgy commented Dec 21, 2019

eterna2 commented Dec 23, 2019 • edited Loading

Summary of changes:

Impacts on other PRs

TODOs (in future PR?)

eterna2 commented Dec 23, 2019 • edited Loading

Bobgy left a comment

Choose a reason for hiding this comment

Bobgy Dec 24, 2019

Choose a reason for hiding this comment

eterna2 Dec 26, 2019 • edited Loading

Choose a reason for hiding this comment

Bobgy commented Dec 24, 2019

eterna2 commented Dec 26, 2019

Bobgy commented Dec 27, 2019

Bobgy left a comment

Choose a reason for hiding this comment

Bobgy commented Dec 27, 2019

eterna2 commented Jan 2, 2020

Bobgy commented Jan 3, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Bobgy Jan 3, 2020

Choose a reason for hiding this comment

eterna2 Jan 5, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 3, 2020

Bobgy commented Jan 3, 2020

Bobgy commented Jan 3, 2020

Bobgy commented Jan 6, 2020

Bobgy commented Jan 6, 2020

eterna2 commented Dec 17, 2019 •

edited by jlewi

Loading

eterna2 commented Dec 23, 2019 •

edited

Loading

eterna2 commented Dec 23, 2019 •

edited

Loading

eterna2 Dec 26, 2019 •

edited

Loading