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

[EC-423] Fix unit tests #3265

Merged
merged 17 commits into from
Aug 11, 2022
Merged

[EC-423] Fix unit tests #3265

merged 17 commits into from
Aug 11, 2022

Conversation

eliykat
Copy link
Member

@eliykat eliykat commented Aug 9, 2022

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Fix a few unrelated issues we were having when running tests.

Code changes

  • move partial jest config from package.json to jest.config.ts. You can use one location or the other, but not both.
  • fix broken import in libs/common/spec/models/domain/encArrayBuffer.spec.ts
  • fix implementation of bitButton. Per this test, setting buttonType = null should default to the secondary type, but this test is currently failing. Fix how we handle the default buttonType.
  • fix Firefox import data - baseImporter was logging parsing errors to the console every time tests were run, presumably from malformed test data. This probably meant that the test was passing for the wrong reason. Replacing this line with fresh (redacted) export data fixes it.

Screenshots

Before you submit

- [x] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

@eliykat eliykat requested a review from a team August 9, 2022 04:29
djsmith85
djsmith85 previously approved these changes Aug 9, 2022
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

@eliykat Thank you for fixing these. I also noticed some broken tests earlier today.

I can approve all change besides the button.directive. I was not part of that initiative. Will leave that up to EC and SSG

@djsmith85 djsmith85 requested a review from a team August 9, 2022 18:48
MGibson1
MGibson1 previously approved these changes Aug 9, 2022
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

I don't like the testEnvironment = node, but I can't see that it hurts anything

jest.config.js Outdated Show resolved Hide resolved
@eliykat eliykat dismissed stale reviews from MGibson1 and djsmith85 via a5ab6d0 August 10, 2022 03:24
@eliykat
Copy link
Member Author

eliykat commented Aug 10, 2022

I've made the following additional changes. This is all to enable #3216 so that we can have unit tests run in the pipeline again.

  • create base jest config which all projects can extend, to minimise duplication
  • add .spec files to web tsconfig so that we have proper path mapping (enabling the @bitwarden/common imports)
  • fix a few more tests which have been broken on master
  • add web to the root jest.config so that it's included when all tests are run
  • undo the testEnvironment change @MGibson1 commented on (I discussed with Vince)
  • add maxWorkers: 3 setting, see comment for explanation

I've forked this and merged #3216 into it to confirm that it successfully runs all tests: https://github.com/eliykat/bitwarden-clients/runs/7759609192?check_suite_focus=true

@eliykat eliykat requested review from MGibson1 and djsmith85 August 10, 2022 04:16
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

I'm concerned about including spec and test setup files in tsconfig. I think webpack should always treeshake these out in production builds, but I'm not 100% confident in my knowledge of that witchcraft.

regardless, we have tsconfig.spec.json to handle these -- does that solution not work for some reason? Jest should be honoring those...

apps/cli/jest.config.js Show resolved Hide resolved
apps/browser/tsconfig.json Outdated Show resolved Hide resolved
Comment on lines 22 to 24
"./test.setup.ts"
],
"include": ["src/connectors/*.ts", "src/**/*.stories.ts", "src/**/*.spec.ts"]
Copy link
Member

Choose a reason for hiding this comment

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

we have tsconfig.spec.json to configure test includes in, I don't think we want to include any specs or test setups in the base tsconfig files.

Happy to be wrong about that, but I don't want tests in our prod outputs and I'm not sure they won't be if they are included here.

libs/angular/tsconfig.json Outdated Show resolved Hide resolved
libs/common/jest.config.js Show resolved Hide resolved
libs/common/tsconfig.json Outdated Show resolved Hide resolved
libs/components/tsconfig.json Outdated Show resolved Hide resolved
libs/node/jest.config.js Show resolved Hide resolved
@eliykat
Copy link
Member Author

eliykat commented Aug 11, 2022

I've had some wins and losses here 😅

Win: ts-jest didn't know to use tsconfig.spec.json. Once I added that to the base jest config, I could move files: ["./test.setup.ts"] into tsconfig.spec.json and it still works. This is required to make sure that the setup gets compiled and run when running tests, but at least it gets it out of the main tsconfig.json.

Lose: as discussed, we need to include the .spec code in the TS compilation to get path mapping in the IDE, but VsCode doesn't recongise tsconfig.json by any other name. e.g. see here, here. So we do need to add it to the main tsconfig.json, and we already do this in libs/common without a problem. I inspected the webpack bundle for web on this branch using webpack-bundle-analyzer and this change didn't cause any spec files to be added. Which makes sense, because there are no entry points to that code.

After these changes, web is the only app to have .spec files added to tsconfig, so I haven't checked the others. This seems safe for now though.

All your feedback should be addressed.

@eliykat eliykat requested a review from MGibson1 August 11, 2022 01:03
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@eliykat eliykat merged commit 4a1c3eb into master Aug 11, 2022
@eliykat eliykat deleted the EC-423-fix-unit-tests branch August 11, 2022 01:35
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