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

feat(jest-cypress): add ESLint config and TS support #117

Merged
merged 50 commits into from
Sep 11, 2020

Conversation

IlCallo
Copy link
Member

@IlCallo IlCallo commented Apr 29, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • New test runner
  • Documentation
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

If you are adding a new test runner, have you...? (check all)

  • Created an issue first?
  • Registered it in /packages/base/runners.json?
  • Added it to /README.md?
  • Included one test that runs baseline.spec.vue?
  • Added and updated documentation?
  • Included a recipe folder with properly building quasar project?

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the dev branch and not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on Windows
  • It's been tested on Linux
  • It's been tested on MacOS
  • Any necessary documentation has been added or updated in the docs (for faster update click on "Suggest an edit on GitHub" at bottom of page) or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
@webnoob and @outofmemoryagain
Up for you to beta-test!
Testing on Windows and Mac is missing.

Gonna work on creating a better (and type-safe) mountQuasar tomorrow

Closes #99
Closes #95
Partially address #91 (just Jest and Cypress)
Closes #48 (doesn't actually cover all harnesses, but most of Jest related stuff there has been integrated)

@Evertvdw
Copy link
Contributor

Hi there! I'm also using Quasar testing with Typescript, good that this is becoming part of the extension :)

In the jest.config.js I exclude collecting coverage from index/types.ts files as well as '.d.ts' files, because that clutters the resulting coverage file. Maybe include that in here as well?
"<rootDir>/src/**/{!(types|index|*.d),}.ts"

Second we exclude the /src-ssr/ and /src/boot/ folders from the coverage, for that same reason, maybe add exclusions for other build modes as well?

	coveragePathIgnorePatterns: [
		"<rootDir>/node_modules/",
		"<rootDir>/src/boot/",
		"<rootDir>/src-ssr/"
	],

And finally I have a question, why is the utils file still a .js and not a .ts? Is there a good reason for it? Because I'm using .ts files and maybe I shouldn't. Speaking of utils a moduleNameMapper for them could also be useful:
"^utils/(.*)$": "<rootDir>/test/jest/utils/$1",

That was it for now, hope it's useful!

@IlCallo
Copy link
Member Author

IlCallo commented May 12, 2020

Hey there @Evertvdw! Nice suggestions, gonna integrate some in next beta release.

Some comments:

  • it totally makes sense to exclude .d.ts files
  • I'm not really sure about index.ts, they usually are barrel files, but not always
  • I don't remember what types.ts should be, seems normal file name to me
  • /src-ssr/ I agree all modes should be excluded, until we can manage those files with TS too (but it doesn't really make sense in some cases)
  • /src/boot why are you excluding those? They contain important code, I think they should be checked by the coverage
  • why is the utils file still a .js and not a .ts? Because mountQuasar util is tricky to type correctly and I didn't want to delay the current beta to include it, the TS version will probably be included in following beta releases
  • mapping test/jest/utils is ok for tests (I played a bit with the idea), but require a corresponding TS alias. I'd prefer to avoid this as its trivial to add and the less we touch the project tsconfig.json, the better

@Evertvdw
Copy link
Contributor

@IlCallo

  • We use types.ts a lot in our project, but I guess that it is not a general way of working :)
  • Index.ts could contain code indeed, it probably depends on how you structure your code. In our project we have a lot of boilerplate in there so including it clutters coverage results.
  • I exclude boot files because we don't unit test those. Mostly stuff we set up there we have to mock in unit tests and Unit-testing a boot file itself is kind of a hassle I think. But I could be wrong.

Regarding mountQuasar, we are using a similar function extensively but have quite some trouble with seperating vueInstance and using quasar on them. Make sure you also create a test file in which you call that function twice and see if everything goes as expected the second time :)

@egyel
Copy link

egyel commented May 15, 2020

These are great features.
I am using Quasar with TypeScript. How could I use this even before the merge?

@IlCallo
Copy link
Member Author

IlCallo commented May 15, 2020

@egyel it's already out as beta!
Use quasar ext add @quasar/testing-unit-jest@beta

Yet, vue-test-utils has been updated last week and I'm afraid this broke something, I still have to check and release a new beta

@IlCallo
Copy link
Member Author

IlCallo commented May 25, 2020

@Evertvdw I checked again, current setup only collect coverage from src/** folder so no need to exclude feature specific folders.

I'll exclude .d.ts tho, adding them to coveragePathIgnorePatterns array.

@IlCallo
Copy link
Member Author

IlCallo commented May 25, 2020

Updated the beta to adapt to vue-test-utils stable release and to ignore .d.ts files

@IlCallo IlCallo changed the title feat(jest): add ESLint config and TS support feat(jest-cypress): add ESLint config and TS support May 27, 2020
@IlCallo IlCallo mentioned this pull request Jul 8, 2020
23 tasks
@Blfrg
Copy link
Contributor

Blfrg commented Sep 6, 2020

Coming from the mountQuasar discussion but shifting focus to Cypress;

Looking further into this pull request it looks like you've got everything covered already!
You've already resolved the Cypress + Jest type conflicts I was going to bring up.

Though I wonder if you get the same type conflicts I do when running quasar dev or quasar build

WAIT  Compiling...                                                                                                                                                                                                                                         6:11:12 PM



 • Compiling:
 └── SPA ████████████████████ 100% done in 407 ms



 DONE  Compiled successfully in 416ms                                                                                                                                                                                                                       6:11:12 PM


 N  App dir........... /project-dir
    App URL........... http://localhost:8080
    Dev mode.......... spa
    Pkg quasar........ v1.13.2
    Pkg @quasar/app... v2.0.9
    Transpiled JS..... yes (Babel) - includes IE11 support

ERROR in /project-dir/test/cypress/integration/home.spec.ts(143,3):
TS2304: Cannot find name 'cy'.
ERROR in /project-dir/test/cypress/integration/home.spec.ts(161,1):
TS2304: Cannot find name 'context'.
ERROR in /project-dir/test/cypress/integration/home.spec.ts(200,3):
TS2304: Cannot find name 'specify'.
// and on and on..

It seems fork-ts-checker-webpack-plugin doesn't realize there's multiple .eslintrc.js
I even tried to set it to false in quasar.conf.js

  supportTS: {
    tsLoaderConfig: {},
    tsCheckerConfig: {
      eslint: false,
    },
  },

which hides the errors on initial compile, but it still lints after modifying Vue components and recompiling


I just tried to start from scratch to verify:

quasar create starter
quasar ext add @quasar/testing
quasar ext add @quasar/testing-unit-jest@beta <-- 1.1.0-beta.6
quasar ext add @quasar/testing-e2e-cypress@beta <-- 1.0.0-beta.13

all with typescript support and minimal defaults.

I wanted to note that the unit-jest install gave this error:

? Please choose how to install required babel rules: Overwrite babel.config.js and use additional .babelrc
? Jest Unit testing will now be installed. Please choose additional options: Extra "scripts" in your package.json, TypeScript support, SFC webpack <test> loader

 App · Updating /quasar.extensions.json for "@quasar/testing-unit-jest" extension ...
 App · Running App Extension install script...

 App · ⚠️  Extension(@quasar/testing-unit-jest): extendJsonFile() - "/starter/tsconfig.json" doesn't conform to JSON format: this could happen if you are trying to update flavoured JSON files (eg. JSON with Comments or JSON5). Skipping...
 App · ⚠️  Extension(@quasar/testing-unit-jest): extendJsonFile() - The extension tried to apply these updates to "/starter/tsconfig.json" file: {"compilerOptions":{"esModuleInterop":true,"types":["quasar","jest"]}}

I had to modify tsconfig.json manually even though it didn't appear to have any conflicts.

I confirmed that among other [default] lint warnings I do see cypress type warnings when running quasar dev or quasar build

ERROR in /starter/test/cypress/integration/home.spec.ts(6,5):
TS2304: Cannot find name 'cy'.
ERROR in /starter/test/cypress/plugins/index.ts(17,29):
TS2694: Namespace 'Cypress' has no exported member 'PluginConfig'.
ERROR in /starter/test/cypress/support/commands.ts(34,28):
TS2315: Type 'Chainable' is not generic.
ERROR in /starter/test/cypress/support/commands.ts(60,1):
TS2708: Cannot use namespace 'Cypress' as a value.
ERROR in /starter/test/cypress/support/commands.ts(65,24):
TS7006: Parameter 'loc' implicitly has an 'any' type.
ERROR in /starter/test/cypress/support/commands.ts(66,22):
TS2339: Property 'to' does not exist on type 'JestMatchersShape<Matchers<void, any>, Matchers<Promise<void>, any>>'.
ERROR in /starter/test/cypress/support/commands.ts(74,1):
TS2708: Cannot use namespace 'Cypress' as a value.
ERROR in /starter/test/cypress/support/commands.ts(81,1):
TS2708: Cannot use namespace 'Cypress' as a value.
ERROR in /starter/test/cypress/support/index.ts(21,1):
TS2708: Cannot use namespace 'Cypress' as a value.
ERROR in /starter/test/cypress/support/index.ts(21,35):
TS7006: Parameter 'err' implicitly has an 'any' type.

But again that may be more related to quasar itself, not testing
except that it pertains to cypress types using a local .eslintrc.js and local tsconfig.json

I haven't found any way around this yet.

@IlCallo
Copy link
Member Author

IlCallo commented Sep 7, 2020

@Blfrg those errors where due to some problem I already fixed but not yet released.
I just pushed a new beta version of both Jest and Cypress harness, the Cypress one should solve global typings conflicts, please check it out and tell me if you still get some errors.

I wanted to note that the unit-jest install gave this error:

That error is generated when your tsconfig.json contains comments or other JSON flavour stuff (eg. leading commas), as stated by the error itself. Can you check if this was the case?

@Blfrg
Copy link
Contributor

Blfrg commented Sep 7, 2020

That error is generated when your tsconfig.json contains comments or other JSON flavour stuff (eg. leading commas), as stated by the error itself. Can you check if this was the case?

Here is a fresh quasar-starter-kit with selected options:

> quasar create starter

  ___
 / _ \ _   _  __ _ ___  __ _ _ __
| | | | | | |/ _` / __|/ _` | '__|
| |_| | |_| | (_| \__ \ (_| | |
 \__\_\\__,_|\__,_|___/\__,_|_|



? Project name (internal usage for dev) starter
? Project product name (must start with letter if building mobile apps) Quasar App
? Project description A Quasar Framework app
? Author Blfrg
? Pick your favorite CSS preprocessor: (can be changed later) Stylus
? Pick a Quasar components & directives import strategy: (can be changed later) Auto import
? Check the features needed for your project: ESLint (recommended), TypeScript, Vuex, Axios, Vue-i18n, IE11 support
? Pick a component style: Composition
? Pick an ESLint preset: Prettier
? Continue to install project dependencies after the project has been created? (recommended) NPM

  Quasar CLI · Generated "starter".

Here is the resulting tsconfig.json

{
  "extends": "@quasar/app/tsconfig-preset",
  "compilerOptions": {
    "baseUrl": ".",
    "target": "es5",
  }
}

Here is adding quasar-testing
Note: I skip Jest+Cypress since I will install the beta versions after, but the base quasar-testing extension is required

> quasar ext add @quasar/testing
? Please choose which testing harnesses to install: Security Anti-Vulnerability

tsconfig.json is still unmodified:

{
  "extends": "@quasar/app/tsconfig-preset",
  "compilerOptions": {
    "baseUrl": ".",
    "target": "es5",
  }
}

Now I add unit-jest@beta

> quasar ext add @quasar/testing-unit-jest@beta

? Please choose how to install required babel rules: Overwrite babel.config.js and use additional .babelrc
? Jest Unit testing will now be installed. Please choose additional options: Extra "scripts" in your package.json, TypeScript support, SFC webpack <test> loader

 App · Updating /quasar.extensions.json for "@quasar/testing-unit-jest" extension ...
 App · Running App Extension install script...
(node:733526) UnhandledPromiseRejectionWarning:   SyntaxError: /starter/tsconfig.json: Unexpected token } in JSON at position 112

  - parse

  - loader.js:1171 Object.Module._extensions..json
    internal/modules/cjs/loader.js:1171:22

  - loader.js:985 Module.load
    internal/modules/cjs/loader.js:985:32

  - loader.js:878 Function.Module._load
    internal/modules/cjs/loader.js:878:14

  - loader.js:1025 Module.require
    internal/modules/cjs/loader.js:1025:19

  - helpers.js:72 require
    internal/modules/cjs/helpers.js:72:18

  - install.js:91
    [starter]/[@quasar]/quasar-app-extension-testing-unit-jest/src/install.js:91:24

  - Array.forEach

  - install.js:58 module.exports
    [starter]/[@quasar]/quasar-app-extension-testing-unit-jest/src/install.js:58:23

  - Extension.js:339 Extension.__runInstallScript
    [starter]/[@quasar]/app/lib/app-extension/Extension.js:339:11


(node:733526) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:733526) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

The error was much worse this time but helped lead me to a solution.
I had to remove the trailing comma from the existing tsconfig.json character 112

{
  "extends": "@quasar/app/tsconfig-preset",
  "compilerOptions": {
    "baseUrl": ".",
    "target": "es5" <-- removed comma
  }
}

I was able to complete the unit-jest@beta install after making this change.

That tsconfig.json is generated by the quasar-starter-kit
and is likely the path a newcomer would take to get started, so it should be corrected by default.
I'll submit a PR to correct the default there.
But since the issue could still occur with custom code bases, an alternative might still need to be considered.


I just pushed a new beta version of both Jest and Cypress harness, the Cypress one should solve global typings conflicts, please check it out and tell me if you still get some errors.

To further confirm e2e-cypress@beta installed without issue.
I tested the defaults and I'm still seeing cypress type errors in the console:

> npm run test:e2e
// ...
ERROR in /starter/test/cypress/support/index.ts(22,7):
@typescript-eslint/no-unsafe-call: Unsafe call of an any typed value.
ERROR in /starter/node_modules/cypress/types/bluebird/index.d.ts(790,32):
TS2304: Cannot find name 'IterableIterator'.
ERROR in /starter/node_modules/cypress/types/chai/index.d.ts(840,49):
TS2304: Cannot find name 'ReadonlySet'.
ERROR in /starter/node_modules/cypress/types/chai/index.d.ts(840,66):
TS2304: Cannot find name 'ReadonlyMap'.
ERROR in /starter/test/cypress/support/index.ts(22,19):
TS2339: Property 'startsWith' does not exist on type 'string'.
// ... duplicates removed for brevity

Note: Once quasar generates the files it clears the screen, but the cypress type errors are already there if you scroll up.
If you modify a .vue file while running npm run test:e2e you will see the cypress type errors again after regenerated.
The type errors do appear different now though, related to bluebird and chai so progress has been made!

I know this is a WIP and difficult to nail down.
Overall it's functional and only reporting false positives, and it looks fine in the IDE.

@IlCallo
Copy link
Member Author

IlCallo commented Sep 8, 2020

I skip Jest+Cypress since I will install the beta versions after, but the base quasar-testing extension is required

quasar-testing isn't really required, it's more of an helper to manage testing harnesses.
I'm taking over the testing scope but I still haven't been able to check all pieces, I still haven't found time to check and fix the "coordinator" package


I merged the trailing comma removal, as it was the source of the error as you correctly identified


I tested the defaults and I'm still seeing cypress type errors in the console:

The last error you reported makes me think the issue is related to you having enabled IE support ("target": "es5"), and adding it in my demo project caused the same errors.

Update test/cypress/tsconfig.json adding "compilerOptions": { "target": "es6" } and everything should run fine.
I'll add an override on the default template, with a comment explaining why it's needed

@Blfrg
Copy link
Contributor

Blfrg commented Sep 10, 2020

@IlCallo Thank you for the merge and the details!

I confirmed the suggested change "compilerOptions": { "target": "es6" }
does correct those remaining cypress[/bluebird/chai] type errors.


When I mentioned "quasar-testing extension is required" I meant in the case that someone uses the following:

> quasar test --unit jest
 App · ⚠️  Quasar App Extension "@quasar/testing" is missing...

> quasar test --e2e cypress
 App · ⚠️  Quasar App Extension "@quasar/testing" is missing...

I normally use npm run test:unit
I wasn't aware of the command until reading through issues the other day, so I started trying it occasionally.
Since I received that error, I mentally noted I should always install @quasar/testing when I create new starters.

On that note, when everything is properly installed;
quasar test --unit jest works with latest beta
quasar test --e2e cypress does not work with latest beta

I assume this has to do with the new script which starts dev+e2e (nice addition BTW!)
Quasar starts dev then idles without starting cypress
This may disrupt those who rely on this command already, or confuse newcomers.

Copy link
Contributor

@Blfrg Blfrg left a comment

Choose a reason for hiding this comment

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

@IlCallo I'm not sure if this review (status: Pending) is visible.
Please consider the proposed changes (Cypress 5.x), thank you.

packages/e2e-cypress/package.json Outdated Show resolved Hide resolved
@IlCallo
Copy link
Member Author

IlCallo commented Sep 10, 2020

I'm taking over the testing scope but I still haven't been able to check all pieces, I still haven't found time to check and fix the "coordinator" package

Unluckily I can only quote myself... Taking over a codebase written by someone else takes time :)
I see you pretty active in this field, we happily welcome new contributors over Quasar ecosystem if you feel so

@IlCallo
Copy link
Member Author

IlCallo commented Sep 10, 2020

Beta docs live at https://ilcallo.github.io/quasar-testing/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants