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

test: add initial unit test suite #321

Merged
merged 29 commits into from
May 10, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented May 8, 2022

Summary

Adds an initial batch of unit tests, heavily updated and refactored from #135 (and several new tests added on top as well). Also adds tests to the CI process.

Description

  • deps: add jest for testing, ts-jest for processing TS files, and @jest/globals for importing from Jest within tests

    • add a jest.config.js with necessary configurations
      • ignore fixtures/ dir, only match .spec.ts files for tests (no test helper files)
    • gitignore the coverage directory
  • test: adds unit tests into __tests__ dir

    • Adds unit tests for check-tsconfig, context, diagnostics-format-host, get-options-override, host, nocache, rollingcache, and rollupcontext
      • 100% coverage for all but get-options-overrides; couldn't get expandIncludeWithDirs to work, see in-line comment
    • Missing tests for print-diagnostics, tscache, and index
      • index really still needs heavy integration testing with the Rollup API
      • No need for tests for interface files -- could add type tests if you really wanted to though
      • tslib and tsproxy are workaround/hacky files, but may be able to test them too
        • EDIT: tsproxy is now tested indirectly as it's needed in a few of the specs
  • test: uses __tests__/__temp/ dir for generated test files

    • this is removed after testing and also .gitignore'd
  • lint: adds __tests__ dir to lint dirs

  • ci: adds npm run test:coverage to steps

  • ci: add Node 14, 16, 18, and macOS, Windows to matrix

  • docs: adds a testing section to CONTRIBUTING.md

    • and generally adds sections to split up "Developing"

Review Notes

  • Could use a jest.config.ts if we add ts-node as a dep. I thought you may not want to add that as a dep so I left it out for now

Next Steps

  1. Can add Coverage Reporting and a badge to the README with something like codecov. I use this in my own libraries, but this requires manual set-up so I didn't add it in the code yet. I might have permissions to do it myself now, but not sure; also not sure if you'd want it on my account vs. yours etc

  2. Still need plenty of integration testing for the plugin itself / index. Should use the Rollup API for these

    • Adding common errors and edge cases (e.g. type-only imports, monorepos, etc) as regression tests would be great in particular
    • In general, this would make repros a lot easier and within the test system
    • EDIT: See test: add initial integration test suite #371
  3. Can optimize the above as well as existing filesystem tests by using an in-memory filesystem / tmpfs, e.g. memfs or with a RAMDisk

  4. Improve coverage to 100% of already tested files

  5. Add unit tests for print-diagnostics, tscache, maybe others

@agilgur5 agilgur5 added the scope: tests Tests could be improved. Or changes that only affect tests label May 8, 2022
@agilgur5 agilgur5 requested a review from ezolenko May 8, 2022 17:12
@agilgur5 agilgur5 added the kind: feature New feature or request label May 8, 2022
@agilgur5 agilgur5 mentioned this pull request May 8, 2022
@agilgur5
Copy link
Collaborator Author

agilgur5 commented May 8, 2022

Ack, this merge conflicts with at least one of my recently merged PRs: #319 . I'll fix that and add running tests to the CONTRIBUTING.md from #313 in a bit

__tests__/get-options-overrides.spec.ts Show resolved Hide resolved
src/check-tsconfig.ts Outdated Show resolved Hide resolved
brekk and others added 19 commits May 9, 2022 13:23
modifications made by agilgur5 to brekk's original commit:
- rebase with `master` that is 3 years newer
  - fix conflicts with newer code, esp the `tsModule`/`tsProxy` changes
    in this commit
- move `jest` config to the bottom and don't reformat `package.json`
  with tabs
modifications made by agilgur5 to brekk's original commit:
- fix merge conflicts with code that is 3 years newer
- and ts-jest to v28
- @types/jest doesn't have a v28 yet

- look ma, no more vulns!
- most of these options are either unused (like `modulePaths` etc) or
  covered by Jest's defaults (`moduleFileExtensions`, `testMatch`, etc)

- also use `ts-jest` preset per its docs and migration docs
  - https://kulshekhar.github.io/ts-jest/docs/migration
- per ezolenko's request, though I also prefer to separate them

- fix all relative imports to use `../src/` now

formatting changes:
- improve ordering of imports in tests -- externals first, newline, then
  internal imports
  - do this **consistently**
- consistently put spaces around braces in destructured imports
  - really need better linting (and update to ESLint)
- consistently add newline after imports and before declarations

- in general, add more newlines for readability in several files
- no config needed and uses standard imports too
  - also required for future Jest ESM support I believe
  - I created the `jest-without-globals` package before this feature was
    released in Jest core and contributed a PR for it too, so might have
    a bias toward not using globals
- basically to match the changes I made when fixing declaration map
  sources ~1.5 years ago in ec0568b
- also to add `cwd` to options

- fix: use `toStrictEqual` everywhere, in order to actually check
  `undefined` properties, as this is necessary for some of the checks
  - I believe the original author may have intended to actually check
    `undefined` given that they wrote it in the test, but maybe did not
    know to use `toStrictEqual` instead of `toEqual`
- instead of re-writing the options multiple times, write up some
  defaults and use object spread
  - and refactor `makeDefaultConfig()` to just use
    `{ ...defaultConfig }` instead to match the rest

- re-write normalizePaths to be a `for ... of` loop instead of using
  `Array.map`
  - simpler / more straightforward than a function with side-effects

- feat: add typings in several places
  - that's why we're using TS and ts-jest right??
- LanguageServiceHost now requires 3rd argument, cwd, so add that to
  all usage
- add afterDecalarations to transformers

- Jest's test.skip can now use async/await, so do that
  - it was also giving a type error, and this is simpler anyway

- change expected to use `__tests__` directory now

- fix: use `expect.arrayContaining` for `host.getDirectories` test as
  there are more temp directories that get added here, such as
  `build-self` and `coverage`
  - in general, might be less fragile if this used a generated directory
    instead of an actual one
    - but save bigger refactors for later
- instead of using Promises with `done()`, use async/await for clearer
  code and less indentation too

- remove the `truncateName` function as it's not needed; `local` will
  result in the same name

- remove commented out test code
  - this seemed to just be there for testing the code, and not
    ready-for-production comments
- basically using pre-defined fixture vars instead of ones defined
  inside the test
  - which is more straightforward, easier to read, and less fragile
  - shorter names too

- also use a __temp dir for auto-generated files instead of creating
  them in the same dir and confusing the editor file tree and potential
  watchers
  - change jest config to make sure only spec files are being watched
  - gitignore __tests__/__temp/ dir in case tests crash etc
- use PluginContext and IContext types instead of `any`
  - we're using TypeScript right??
  - add `debug` level logging in a few places where it was missing

- update stubbedContext to have latest PluginContext properties
  - watcher isn't in there anymore and a few new properties were added
- fix type errors with stubbedContext
  - give it an intersection with IContext for `info` and `debug`
    verbosity levels
  - force the logging funcs to `any` as they don't quite match the
    Rollup types
  - force them to `any` when deleting them as well because they're not
    optional properties

- Note: would be good to not force so much `any` if possible, but this
  may be difficult without more advanced internal Rollup mocks
  - couldn't find any testing packages for this :/

- test: add verbosity expect for debug too
- refactor: context2 -> context
  - there wasn't another one, so just use the same name consistently
    - I'm guessing there was another one at some point in the
      development of this and then it was removed but not renamed
- surprisingly only in jest.config.js?

- really need to update to @typescript-eslint ...
- run after all the builds for now
  - it can be done in parallel as a separate job, but then have to
    duplicate the NPM install etc, so may not be optimal that way

- refactor: add test:watch and test:coverage scripts
  - shortcut scripts so don't have to `npm test -- --coverage" etc
  - also remove `--verbose` from `test` as that's not necessary
- increases coverage of get-options-overrides significantly
- couldn't figure out `rootDirs` -- either the code or my tests are
  wrong, so just skip that one for now

- refactor: move makeStubbedContext etc into a shared fixtures dir so
  that it can be used for both rollupcontext tests _and_
  createFilter tests
  - in my stylistic opinion, I prefer to put nearly _all_ consts like
    these into fixtures dir
  - configure jest to ignore test helpers in coverage reporting such as
    fixture files

- format: '' -> "", add semicolon in some places
  - I use single quotes and no semicolons, so missed that in a few
    places
    - lint didn't check for that and no prettier auto-format :/
- basically using pre-defined fixture vars instead of ones defined
  inside the test
  - which is more straightforward, easier to read, and less fragile
  - shorter names too
  - left a couple of ones as is where they were only used once very
    quickly -- could make them fixture vars too but 🤷

- use async/await instead of Promises with `done()` etc
- also use more `fs-extra` functions that support Promises instead of
  synchronous `fs` functions (e.g. `existsSync` -> `pathExists`)
  - async should be a small optimization for tests too

- fix: use __temp dir for auto-generated files instead of creating
  them in a fixtures dir and breaking actual fixtures

- format: a few multi-line statements were able to be condensed to a
  single line, so do so
  - esp as multi-line was not helping readability since this is just
    irrelevant test data (may have hampered readability actually)
- goes over the different ways of running the tests (watch + coverage)
- line about unit and integration tests was moved into this section
  - and altered to reflect the current state of the repo now that a good
    amount of unit tests exist

- also move "Linting and Style" into its own section with a list
- move "fix any failed PR checks" to the top as overall guidance on
  making a PR
  - previously it was in the middle of linting and style, which felt a
    bit disjointed (maybe made sense earlier before builds were in CI?)

- name the last section "Building and Self-Build"; leave content as is
  - well mostly, change the first line as "fastest way to test" is not
    necessarily accurate anymore now that there are actual tests
jest.config.js Show resolved Hide resolved
__tests__/context.spec.ts Outdated Show resolved Hide resolved
- original author, brekk, had made these changes, likely because without
  them, the tests would throw in the source lines where `tsModule` was
  used with things like "Cannot read property 'ModuleKind' of undefined"
  - the actual fix for this is to instead use `setTypescriptModule` from
    tsproxy as this is how it is used in the source
    - it's mainly needed for when an alternate TS is substituted
    - brekk probably didn't know the codebase well enough to know that
    - add `setTypescriptModule` to all specs that need it

- 100% test coverage of tsproxy now too!

- this should hopefully fix the build errors we were getting as well
@agilgur5
Copy link
Collaborator Author

Can add Coverage Reporting and a badge to the README with something like codecov. I use this in my own libraries, but this requires manual set-up so I didn't add it in the code yet. I might have permissions to do it myself now, but not sure; also not sure if you'd want it on my account vs. yours etc

It doesn't seem like I have permissions to add Coverage Reporting myself; think you have to be repository "owner" or otherwise have access to the "Settings" panel of the repo, which I don't have access to.

@agilgur5 agilgur5 deleted the test-add-unit-tests branch July 2, 2023 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: tests Tests could be improved. Or changes that only affect tests topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants