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 typings for mocha #30190

Closed
wants to merge 10 commits into from
Closed

add typings for mocha #30190

wants to merge 10 commits into from

Conversation

vitalics
Copy link
Contributor

@vitalics vitalics commented Feb 6, 2019

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

timroes
timroes previously approved these changes Feb 6, 2019
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes timroes dismissed their stale review February 6, 2019 11:14

Remebered the Jest Mocha typing clash

@timroes
Copy link
Contributor

timroes commented Feb 6, 2019

@elastic/kibana-platform I am not sure if adding mocha types works like that? Jest typings already define a describe and it on the top level, so if we pull in mocha typings those will suddenly collide. I think we would somehow need to make sure that the jest typing is only valid for jest files and the mocha typing for mocha types. Any idea how this can be achieved?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes requested a review from a team February 6, 2019 15:28
"target"
]
}
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the other config files were updated to use different line endings, please update your git config to preserve the line endings that were in the file: https://help.github.com/articles/dealing-with-line-endings/#global-settings-for-line-endings

Copy link
Contributor

Choose a reason for hiding this comment

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

I also recommend installing the editorconfig plugin for your editor so it will automatically pickup configuration files like https://github.com/elastic/kibana/blob/master/.editorconfig

spalger
spalger previously requested changes Feb 8, 2019
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Please supply some description for the intention behind this change, best I can tell this has no effect other than listing @types/mocha in the root package.json when it's not necessary.

package.json Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger
Copy link
Contributor

spalger commented Feb 8, 2019

@vitalics comment here with a little bit more information about your local setup and I'll try to reproduce it locally to see how we can get those errors gone from your setup, but I'm 90% sure we don't need this unnecessary dependency and the tsconfig changes to accomplish it, so I'm closing this PR

  • What branch are you developing on?
  • Are you developing plugins? Can you share the source?
  • What editor are you using?
  • Do you have any Typescript specific editor configuration/plugins?

@spalger spalger closed this Feb 8, 2019
@vitalics vitalics mentioned this pull request Feb 14, 2019
7 tasks
@spalger
Copy link
Contributor

spalger commented Feb 15, 2019

Okay, seems like this is something we do need really soon, so lets go ahead and get this merged.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger dismissed their stale review February 15, 2019 18:26

Working to get this merged by using it in the kibana functional tests

@spalger spalger requested a review from timroes February 15, 2019 18:29
@elasticmachine
Copy link
Contributor

💔 Build Failed

"extends": "../tsconfig.json",
"compilerOptions": {
"types": [
"@kbn/test/types/expect.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

@timroes the expect.js types from npm define a global expect variable, which I assume is important for some usages of the library, but is colliding poorly in the Kibana repo where expect.js and jest need to coexist. Since we are somewhat special in this regard and expect.js is effectively done, I copied the type definition into @kbn/test, changed it to a simple module type, and referenced it from the relevant config files. It won't have the same autoloading benefits of a @types/ package, but I think it's a suitable solution for the problem. Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a good behavior for now, and I currently don't see a reason why it should be bad.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v8.0.0 v7.2.0 labels Feb 17, 2019
export default function ({ getService, loadTestFile }) {
import { FtrProviderContext } from '../../../types';

// tslint:disable-next-line no-default-export
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we perhaps disable this check completely for the function tests, since it's just the way the functional test framework has been build and even new test files will have to use that, so I think we should rather disable it in tslint for the test folder than to force every file to have that tslint disable in it, since I think an inline tslint-disable should be for exceptional cases, and ideally should go with a comment why it needs to be disabled there. This is not an exceptional case, but the expected behavior.

Copy link
Contributor

@spalger spalger Feb 19, 2019

Choose a reason for hiding this comment

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

Not really, tslint doesn't support overrides for patterns or anything like eslint does.

When https://github.com/typescript-eslint/typescript-eslint is ready and we move to it we will be able to, but until then this is our best option without dropping tslint.yaml files all over the place with I think is even worse. palantir/tslint#3447 (comment)

@vitalics
Copy link
Contributor Author

vitalics commented Feb 25, 2019

I suppose we need to close this PR since #31234 is already merged and closed

@vitalics vitalics closed this Feb 25, 2019
spalger pushed a commit that referenced this pull request Feb 28, 2019
…onfig files (#31948)

In #31234 there were some extra changes that I've reverted, like use of the `tsconfig-paths` package to magically rewrite import statements to defy the standard node module resolution algorithm, the inclusion of several unnecessary options in the `test/tsconfig.json` file, and changes of the line-endings in the config files. This also brings a few enhancements from #30190 including a modularized version of the expect.js types, and options for explicit mappings for the PageObjects and services used in ftr tests.
spalger pushed a commit to spalger/kibana that referenced this pull request Feb 28, 2019
…onfig files (elastic#31948)

In elastic#31234 there were some extra changes that I've reverted, like use of the `tsconfig-paths` package to magically rewrite import statements to defy the standard node module resolution algorithm, the inclusion of several unnecessary options in the `test/tsconfig.json` file, and changes of the line-endings in the config files. This also brings a few enhancements from elastic#30190 including a modularized version of the expect.js types, and options for explicit mappings for the PageObjects and services used in ftr tests.
spalger pushed a commit that referenced this pull request Feb 28, 2019
…onfig files (#31948) (#32250)

In #31234 there were some extra changes that I've reverted, like use of the `tsconfig-paths` package to magically rewrite import statements to defy the standard node module resolution algorithm, the inclusion of several unnecessary options in the `test/tsconfig.json` file, and changes of the line-endings in the config files. This also brings a few enhancements from #30190 including a modularized version of the expect.js types, and options for explicit mappings for the PageObjects and services used in ftr tests.
spalger pushed a commit to spalger/kibana that referenced this pull request Mar 9, 2019
…onfig files (elastic#31948)

In elastic#31234 there were some extra changes that I've reverted, like use of the `tsconfig-paths` package to magically rewrite import statements to defy the standard node module resolution algorithm, the inclusion of several unnecessary options in the `test/tsconfig.json` file, and changes of the line-endings in the config files. This also brings a few enhancements from elastic#30190 including a modularized version of the expect.js types, and options for explicit mappings for the PageObjects and services used in ftr tests.
# Conflicts:
#	src/functional_test_runner/lib/config/schema.js
#	test/common/services/es.ts
#	test/functional/page_objects/index.ts
#	test/functional/services/apps_menu.js
#	yarn.lock
spalger pushed a commit to spalger/kibana that referenced this pull request Mar 9, 2019
…onfig files (elastic#31948)

In elastic#31234 there were some extra changes that I've reverted, like use of the `tsconfig-paths` package to magically rewrite import statements to defy the standard node module resolution algorithm, the inclusion of several unnecessary options in the `test/tsconfig.json` file, and changes of the line-endings in the config files. This also brings a few enhancements from elastic#30190 including a modularized version of the expect.js types, and options for explicit mappings for the PageObjects and services used in ftr tests.
# Conflicts:
#	src/functional_test_runner/lib/config/schema.js
#	test/common/services/es.ts
#	test/functional/page_objects/index.ts
#	test/functional/services/apps_menu.js
#	yarn.lock
spalger pushed a commit that referenced this pull request Mar 9, 2019
…32827)

* [ts] add script to verify that all ts is in a project (#32727)

Based on #32705 

We currently have TypeScript code that was backported to 7.0, which was backported without issue because it falls outside of any TypeScript projects in 7.0. This means that the pre-commit hooks break on changes to these files, and that they are not getting type checked by the type_check script. To fix this we need to verify that every typescript file in the repository is covered by a tsconfig.json file as part of CI.

* tests typescript migration (#31234)

* add typescript support for functional tests

* [ts][ftr] improve types for ftr and expect.js, cleanup changes to tsconfig files (#31948)

In #31234 there were some extra changes that I've reverted, like use of the `tsconfig-paths` package to magically rewrite import statements to defy the standard node module resolution algorithm, the inclusion of several unnecessary options in the `test/tsconfig.json` file, and changes of the line-endings in the config files. This also brings a few enhancements from #30190 including a modularized version of the expect.js types, and options for explicit mappings for the PageObjects and services used in ftr tests.
# Conflicts:
#	src/functional_test_runner/lib/config/schema.js
#	test/common/services/es.ts
#	test/functional/page_objects/index.ts
#	test/functional/services/apps_menu.js
#	yarn.lock
@spalger spalger deleted the tests-types branch May 30, 2019 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants