test: add initial integration test suite #371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- some additional ts-jest config is needed to parse `index.ts`
- since Rollup isn't used when importing files during testing, we need to enable `esModuleInterop`
- the import of `findCacheDir` was immediately erroring without this (and maybe more, but `esModuleInterop` made the import work)
- add `tsconfig.test.json` for this purpose
- use `ts-jest` JSDoc types to get types for the ts-jest config as well
- add an integration/ dir in the __tests__/ dir for all integration tests
- add a fixtures/ dir in there as well for integration test fixtures
- add a `tsconfig.json` for all fixtures to use
- basically so that rpt2 source code is not `include`d, though also bc this may be good to customize for fixtures
- add a simple fixture with "no-errors" that does an import and a type-only import
- add a simple integration test that just ensures that all files are part of the bundle
- the bundled `index` file and each file's declaration and declaration map
- and no _other_ files!
- for Rollup, need to use paths relative to the root of the whole project
- probably because that is `cwd` when running `jest` from root
- will probably abstract out helpers here in the future as the test suite grows
- 70% coverage of `index.ts` with just this one test!
- update CONTRIBUTING.md now that an integration test exists
agilgur5
added
scope: docs
- similar to the unit test suite - also remove the `.only` that I was using during development
- should probably do this for each integration test suite
- refactor: split out a `genBundle` func to DRY things up
- this was bound to happen eventually
- don't think testing other output formats is necessary since we don't have any specific logic for that, so that is just Rollup behavior
- take `input` path and rpt2 options as params
- and add the `tsconfig` as a default
- add a hacky workaround so that Rollup doesn't output a bunch of warnings in the logs
- format: use double quotes for strings consistently (my bad, I normally use single quotes in my own repos)
agilgur5
force-pushed
the
test-add-integration-tests
branch
from
b519f93
to
1710a99
Compare
- so we can clean it up after testing to ensure consistency - and so we're not filling up the cache in `node_modules` with testing caches - also add a comment to one the of the tests
- make sure they don't get output when not asked for
- `clean: true` also means that no cache is created - which is a bit confusing naming, but was requested by a few users and implemented in f15cb84 - so have to create the bundle one _more_ time to create the cache - note that `tscache`'s `getCached` and `isDirty` functions are now actually covered in the coverage report
- double it bc it was occassionally failing on CI due to timeouts
- ensures that TS understands the JS w/ DTS w/o error - and that rpt2 filters out JS while Rollup still resolves it on its own (since Rollup understands ESM) - similar to testing w/ a different plugin (i.e. literally testing an "integration"), but this is native Rollup behavior in this case where it doesn't need a plugin to understand ESM - also the first test of actual code contents - reformat the cache test a bit into its own block since we now have ~3 different sets of tests in the suite
- this doesn't test a new code path and the first test already tests the entire bundle for the fixture, so the other tests just repeat that
agilgur5
force-pushed
the
test-add-integration-tests
branch
from
874270e
to
efced2b
Compare
- refactor: rename `integration/index.spec` -> `integration/no-errors.spec`
- refactor: split `genBundle` func out into a helper file to be used by multiple integration test suites
- simplify the `genBundle` within `no-errors` as such
- create new `errors` fixture just with some simple code that doesn't type-check for now
- add test to check that the exact semantic error pops up
- and it even tests colors 😮 ...though I was confused for some time why the strings weren't matching... 😐
- add test to make sure it doesn't pop up when `check: false` or `abortOnError: false`
- when `abortOnError: false`, detect that a warning is created instead
- due to the new `errors` dir in the fixtures dir, the fixtures `tsconfig` now picks up two dirs
- which changes the `rootDir` etc
- so create two tiny `tsconfig`s in both fixture dirs that just extend that one
- and now tests all run simiarly to before
agilgur5
changed the title
- this too timed out, probably due to the checks that _don't_ error
- so that Jest can parallelize these - CI was timing out on this, so splitting it out should help as it'll be 10s each
- bc it was still timing out in some cases 😕 - this time the `no-errors` suite was timing out, so just increase all around
|
I've added a handful more integration tests now, so this is now a proper integration test suite, and not just a simple one-test harness anymore. Edited the title and description of the PR to match. Had to bump test timeouts a couple times since integration tests take a good bit longer, especially when comparing two builds against each other (e.g. for cache / no cache). A few more areas to add tests to:
But this PR is (still) ready to review/merge, those are extras and can be done in future PRs (same as the tests I added today on top of the simple harness from a week ago) |
- woooops... was wondering why it was `.ts`; turns out because I wrote the `output.file` setting that way 😅 😅 😅 - also add a missing comment in errors for consistency - and put code checks _after_ file structure checks, since that's a deeper check
- should output declaration and declaration map for file - code + declaration should contain the one-line function - code _shouldn't_ contain anything from TS files - since this is plain ESM code, we don't need another plugin to process this - nice workaround to installing another plugin that I hadn't thought of till now!
agilgur5
force-pushed
the
test-add-integration-tests
branch
from
60d2e5f
to
172c32f
Compare
- add a file to `errors` with a syntax error in it
- apparently this does throw syntax err, but does not cause `emitSkipped: true`... odd...
- so may need another test for that...
- `abortOnError: false` / `check: false` both cause Rollup to error out instead
- rename `errors/index` -> `errors/semantic` since we have different kinds now
- change the `include` to only take a single file, so that we don't unnecessarily generate declarations or type-check other error files
- refactor(test): rewrite `genBundle` in both files to take a relative path to a file
- simplifies the `emitDeclarationOnly` test as we can now just reuse the local `genBundle` instead of calling `helpers.genBundle` directly
- use the same structure for `errors`'s different files as well
- refactor(test): split `errors.spec` into `tsconfig` errors, semantic errors, and syntactic errors (for now)
- add a slightly hacky workaround to get `fs.remove` to not error
- seems to be a race condition due to the thrown errors and file handles not being closed immediately on throw (seems like they close during garbage collection instead?)
- see linked issue in the comment; workaround is to just give it some more time
- not sure that there is a true fix for this, since an improper close may cause indeterminate behavior
- got a Windows error in CI for the `errors` test suite
agilgur5
force-pushed
the
test-add-integration-tests
branch
from
172c32f
to
62c3b85
Compare
agilgur5
added
topic: OS separators
This was referenced Jul 5, 2022
|
Awesome! |
This was referenced Jul 6, 2022
This was referenced Jul 20, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Creates a test harness/structure for integration tests and adds an initial integration test suite to it
Details
some additional
ts-jestconfig is needed to parseindex.tsesModuleInteropfindCacheDirwas immediately erroring without this (and maybe more, butesModuleInteropmade the import work)tsconfig.test.jsonfor this purposets-jestJSDoc types to get types for thets-jestconfig as welladd an
integration/dir in the__tests__/dir for all integration testsfixtures/dir in there as well for integration test fixturestsconfig.jsonfor all fixtures to useincluded, though also bc this may be good to customize for fixturesadd a simple integration test that just ensures that all files are part of the bundle
indexfile and each file's declaration and declaration mapcwdwhen runningjestfrom rootindex.tswith just this one test!update
CONTRIBUTING.mdnow that an integration test existsEDIT: added more tests below!
allowJs+emitDeclarationOnlyfrom feat: supportemitDeclarationOnly#366tsconfigerrorcheck: falseandabortOnError: falseReview Notes
Creating the harness itself was probably the biggest "mental" blocker for me (mostly in the number of unknowns to solve, but didn't end up being that complex). Now that this is created, I've already started adding more tests to it, namely for edge cases and error cases. Might add some to this PR pending when it gets merged, but otherwise will add those in additional PRs.
Also I haven't quite figured out the error testing process for non-fatal errors yet, but Iwas able to roughly get fix: type-checkincluded files missed bytransform(type-only files) #345 tested with this harness as well, which is one reason why I felt more confident in moving that to "ready for review"