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

Add Integration Tests #627

Merged
merged 11 commits into from
Mar 27, 2020
Merged

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Mar 19, 2020

This is quite a big PR as it refactors a bunch of the test set-up to handle two different types of tests, as well as adds integration tests for tiny-invariant, rollup-plugin-postcss, styled-components, and regeneratorRuntime. Fixes #481

I initially tried to have the integration tests have their own package.json and node_modules, as the build-withConfig folder had before (and with a new CI workflow with working-directory set to the integration test dir), but this proved extremely confusing and very time-consuming to deal with.
It basically requires that scripts, like tests, run from the integration test dir, as if they run from root, they won't read the node_modules (node scans upward, so from the integration test dir, it'll get the integration deps and the TSDX deps, but the opposite doesn't work). And trying to run scripts from there causes lots of inconsistencies all over the place, so I just scrapped that idea.
Open to ideas on better integration testing structures, but the other option proved completely not worth the time, so don't want to spend a ton more time trying to get something else to work either.

Also found some integration failures all over the place while adding this, welp 😕 These include:

  • --extractErrors removes invariant but doesn't replace it with ErrorProd or ErrorDev. And it leaves the import in, just importing nothing 😕
    • warning isn't changed what-so-ever
    • I still think this should be split out as its own Rollup or Babel plugin, it's fairly independent of TSDX, doesn't seem to be used much by users, and can benefit more than just TSDX
  • rollup-plugin-postcss any PostCSS plugins like autoprefixer and cssnano work, but the name of the extracted CSS file is the same as the output, i.e. package-name.cjs.development.css
  • babel-plugin-styled-components doesn't work, plugin order matters for it. See Doesn't play well with babel-plugin-styled-components -- plugin order matters #543

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Mar 23, 2020

Ok fixing the conflicts with #582 in yarn.lock was not easy 😕 . But done now, so this is ready to merge. Should go in before #525 , so need to review this soon

Realized I might be able to use yarn workspaces to have the integration-tests dir split out. Then TSDX can still be used (and as tsdx in scripts), but shared devDeps might not work correctly unless we put them in root similar to Lerna.
I feel like that's really involved prior to having a monorepo (see #634, #635), but that might correctly describe the relationship between TSDX and its integrations; i.e. that it's a separate dependency.

Hmmmmmm... Then again, the relationship between integration tests and E2E tests is actually the same -- they both should be separate workspaces with tsdx used inside of them. So can hold off on this for a bit I guess, since will be refactoring the E2E to be unit tests eventually (and so might do the same for integration?)

Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Ok this looks good to go to me. Made some minor changes for review and fixed a yarn.lock issue resulting from merge conflicts.

This still has the same issue as mentioned in #621 (comment) and the grep issue in #525 (comment) but I think these added tests and fixtures should be merged with urgency given the issues in #638 etc.

Can figure out smaller nuances later. Will leave integration directory as is, maybe can consider extracting as a separate workspace/lerna repo later. Yarn workspaces wouldn't change the dependency tree anyway since they're supposed to all be at root.

test/util.js Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Ok added the grep helper here with credit to the author. But I probably need to refactor the test utils a bit now (see above comment), welp. Was writing up an execWithCache function for this and #621 and that also needs a place to go in utils, so that needs to be done.

Also utils-safePackageName.test.ts is actually a unit test, so that should be put in a unit test directory (and that needs to be added to the README too).

Will get to these in the morning or something

agilgur5 and others added 9 commits March 26, 2020 00:20
- move build-withConfig into integration-tests/fixtures/
  - move its package.json deps into root devDeps
    - it's much easier to work with this way vs. as subdirectories
      with separate package.json and node_modules
      - and consistent with how the E2E tests work too
    - (deps): remove eslint-config-postcss as it wasn't used anyway

- move fixtures/ to test/fixtures as they're specific to E2E tests

- move test/jest.config.json to root jest.config.js
  - so it's used by default, no need for --config
- significantly simplify jest.config.js now that it's in root too
  - and the simplification allows it to be used for both test dirs

(docs): update fixtures README to remove "manual" references
(docs): reword all mentions of tests to say E2E tests
- that's what they currently are, though I'd like to refactor many to
  be unit tests
(docs): add tests README and integration test fixtures README
- the test output strips away `invariant` entirely in all builds,
  instead of replacing it with ErrorProd/ErrorDev etc...
  - it leaves an empty import however...
    - which gives an unused import warning during builds
  - and it doesn't do anything with `warning` at all, it doesn't get
    stripped or changed, and it's message is not in codes.json
    - the source code indeed only checks for 'invariant'
  - and it does generate errors/codes.json, ErrorProd.js, ErrorDev.js
  - I'm not 100% sure, but that seems to be buggy to me

(refactor): split --extractError code into a build-options fixture
- as it doesn't require tsdx.config.js at all
- but it is an integration test, as it requires tiny-invariant etc

(clean): remove the errors/ directory as that's auto-generated by
--extractErrors and is the thing being tested

(clean): remove src/foo.ts as it's extraneous
- add and import a basic CSS file in build-withConfig
  - use the existing tsdx.config.js for the rollup config

- ensure that one CSS file gets extracted at the end
- ensure that it is properly autoprefixed and minifed too
- change testDir in tests to the new names
- reword docs and scripts so they handle this properly

(refactor): interpolate directory in the lint tests
- DRY up so don't have to change all these lines when the path is
  a constant
- because everything other than the test was indeed a fixture, and it's
  better to properly specify those as such for clarity
  - and consistency with the other tests

(format): fix some linting issues in tsdx-lint.test.js
- it's now no longer ignored by the lint script (which ignores the
  whole directory due to the fixtures)
- because it's the only actual unit test!

(docs): add unit test dir to test README
- well, babel-plugin-styled-components fails... but we have a TODO to
  make it work at least!

- ensure styled template tags get converted to regular functions

- add a build-withBabel fixture

(deps): add styled-components
- and it has peerDeps on react-dom and react-is
- add @types/styled-components for TS usage
- when using transform-runtime with helpers: false, it should import
  from @babel/runtime/regenerator
- either this plugin configured like this or a direct import of
  regenerator in one's own library code is the only way to get it to
  be imported
  - just using a generator will make regenerator code added, but
    without any import (leading to a ReferenceError)
  - TODO: make it easier or automatic to use generators
    - useBuiltIns of preset-env?
- regular grep will have an error code if it failed to match, but
  shell.grep doesn't, at least not when silent (maybe it shouldn't
  be silent? that might be too noisy)
  - so add a wrapper that checks that stdout also has the pattern that
    was being matched (grep outputs line:pattern for each match)

Co-Authored-By: Anton Gilgur <agilgur5@gmail.com>
- nothing uses it or its deps and it doesn't seem to have ever been
  used -- the commit that added it (89c0470) didn't use it either
- regression test as there was previously a bug where only plugins
  were merged and presets would only be merged if your preset list
  included preset-env
  - this is the first test that adds any sort of preset

- couldn't find a small preset that would be easy to test against, so
  made a tiny one locally that just does a simple replace with
  babel-plugin-replace-identifiers
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Mar 27, 2020

Copy link
Collaborator Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

create test setup for custom Babel configs
2 participants