Skip to content

[BB2-941] Add Tests CI Check For Sample NodeJS App#11

Merged
JFU-GIT merged 23 commits intomasterfrom
jfuqian/BB2-941-Add-Tests-CI-Check-Sample-NodeJS-App
Mar 22, 2022
Merged

[BB2-941] Add Tests CI Check For Sample NodeJS App#11
JFU-GIT merged 23 commits intomasterfrom
jfuqian/BB2-941-Add-Tests-CI-Check-Sample-NodeJS-App

Conversation

@JFU-GIT
Copy link
Copy Markdown
Contributor

@JFU-GIT JFU-GIT commented Jan 7, 2022

JIRA Ticket:
BB2-941

User Story or Bug Summary:

As BB2 developer and repo maintainer, I need to create tests and a CI check so that I can capture any regression from code changes 

AC:

tests created, covering at least the basic scenario, i.e. start server and client, going through auth and fhir flow without issues.

What Does This PR Do?

What Should Reviewers Watch For?

  • Verify that the CI check is triggered on PR commits
  • Verify linting issues captured by ESLint in pipeline
  • Verify tests executed and passed

If you're reviewing this PR, please check these things, in particular:

What Security Implications Does This PR Have?

Submitters should complete the following questionnaire:

  • If the answer to any of the questions below is Yes, then here's a link to the associated Security Impact Assessment (SIA), security checklist, or other similar document in Confluence: N/A.
    • Does this PR add any new software dependencies? No.
    • Does this PR modify or invalidate any of our security controls? No.
    • Does this PR store or transmit data that was not stored or transmitted before? No.
  • If the answer to any of the questions below is Yes, then please add StewGoin as a reviewer, and note that this PR should not be merged unless/until he also approves it.
    • Do you think this PR requires additional review of its security implications for other reasons? No.

What Needs to Be Merged and Deployed Before this PR?

Comment thread server/src/routes/Authorize.ts Outdated

const router = Router();

// eslint-disable-next-line
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why disabling eslint here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed to more specific disable
// eslint-disable-next-line @typescript-eslint/no-misused-promises

the reason is the router get takes an async func as 2nd parameter - that return a promise where a void is expected.

@nbragdon
Copy link
Copy Markdown
Contributor

I was thinking for the sample app we might just do something lighter weight than jenkins. If it's already setup and working though we can just use that. There are some lightweight ones more built into github that will just run testing commands. I'm thinking we can get away with just mocking and running more unit tests rather than full integration tests. Also have you looked up mocha chai vs. jest in terms of testing framework? I've seen jest become more popular over time and is generally the one you'll see projects recommend.

@nbragdon
Copy link
Copy Markdown
Contributor

I'm also not sure how we are going to get real credentials in here with this method?

@JFU-GIT
Copy link
Copy Markdown
Contributor Author

JFU-GIT commented Jan 21, 2022

I was thinking for the sample app we might just do something lighter weight than jenkins. If it's already setup and working though we can just use that. There are some lightweight ones more built into github that will just run testing commands. I'm thinking we can get away with just mocking and running more unit tests rather than full integration tests. Also have you looked up mocha chai vs. jest in terms of testing framework? I've seen jest become more popular over time and is generally the one you'll see projects recommend.

thanks for feedback, will evaluate jest vs mocha & chai, github stats showing higher usage of jest and may be it is simpler to setup and use.

for using jenkins job for linting and unit testing (may be later selenium tests), the considering is: (1) re-use (2) actively maintained sample apps for now they are node js react, python react, down the road there might be more, node js + react + SDK, etc. and SDK CI/CD could re-use, (3) similar CI/CD which could be managed uniformly via CloudBee jenkins job dashboard.

just some 2c

and it is working, the recent failure is due to some re-factor work in package.json - will fix.

@nbragdon
Copy link
Copy Markdown
Contributor

@JFU-GIT I checked in with Nathan on this and since it is all public infrastructure with essentially no deployments required (at least no deployments to CMS infrastructure) we should be using Github Actions instead of Jenkins. We can then reuse the setup for all sample apps + SDK's going forward.

@JFU-GIT
Copy link
Copy Markdown
Contributor Author

JFU-GIT commented Jan 22, 2022

@JFU-GIT I checked in with Nathan on this and since it is all public infrastructure with essentially no deployments required (at least no deployments to CMS infrastructure) we should be using Github Actions instead of Jenkins. We can then reuse the setup for all sample apps + SDK's going forward.

Make sense, will refactor, thanks for the info.

@JFU-GIT
Copy link
Copy Markdown
Contributor Author

JFU-GIT commented Jan 22, 2022

I'm also not sure how we are going to get real credentials in here with this method?

this is still mocked test (close to unittests), no auth flow (and hence access token involved), only fhir requests logics.

Comment thread client/package.json Outdated
"@typescript-eslint/eslint-plugin": "^5.9.0",
"@typescript-eslint/parser": "^5.9.0",
"axios": "^0.21.2",
"eslint": "^7.25.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A lot of these dependencies we'll probably want to move to dev dependencies. Basically if the library is required while the library/service is up and running then it would be a dependencies. Anything that is part of the development/build process would just be a dev dependency. This helps make the final product a bit smaller.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resolved

Comment thread client/src/index.tsx
@@ -1,3 +1,4 @@
import React from 'react';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did this get added to all these files because of the new linting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes

@nbragdon
Copy link
Copy Markdown
Contributor

@JFU-GIT There seem to be a lot of changes here - and a they don't seem to have to do with testing or setting up github actions. Could you maybe explain what all the changes are in this PR?

@JFU-GIT
Copy link
Copy Markdown
Contributor Author

JFU-GIT commented Feb 17, 2022

@JFU-GIT There seem to be a lot of changes here - and a they don't seem to have to do with testing or setting up github actions. Could you maybe explain what all the changes are in this PR?

the number of source files changed are large (30+?), I think it's because of the linting - i.e. original ts code used tab (4 spaces) as indentation, and linter like 2 spaces indent, when eslint --fix applied, it will impact quite some source files.

others added artifacts are eslint config (.rc) etc.

should be no change to code logic.

Copy link
Copy Markdown
Contributor

@nbragdon nbragdon left a comment

Choose a reason for hiding this comment

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

This seems good, nice work 👍

@JFU-GIT
Copy link
Copy Markdown
Contributor Author

JFU-GIT commented Mar 18, 2022

@nbragdon @dtisza1 did some final touches : use only jest for tests, and note the goal is not linting all the sources for full rules since most of the sources would be gone during the coming soon sdk refactor.
thanks!

@JFU-GIT
Copy link
Copy Markdown
Contributor Author

JFU-GIT commented Mar 18, 2022

Extensive changes to source files *.ts mostly are introduced by running eslint --fix which for example will also change 4 space tab to 2 space tab (by lint rules), think that contribute most of the files impacted.

Copy link
Copy Markdown
Contributor

@dtisza1 dtisza1 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 good to me!

Good work on this!

@JFU-GIT JFU-GIT merged commit efcc50e into master Mar 22, 2022
@JFU-GIT JFU-GIT deleted the jfuqian/BB2-941-Add-Tests-CI-Check-Sample-NodeJS-App branch March 22, 2022 14:46
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.

3 participants