-
Notifications
You must be signed in to change notification settings - Fork 894
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
Chore/migrate mocha tests #1553
Chore/migrate mocha tests #1553
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1553 +/- ##
==========================================
- Coverage 68.08% 67.52% -0.57%
==========================================
Files 3072 3067 -5
Lines 59015 58971 -44
Branches 8924 8944 +20
==========================================
- Hits 40183 39822 -361
- Misses 16645 16971 +326
+ Partials 2187 2178 -9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! no more Mocha! B)
@@ -120,7 +120,8 @@ describe('opensearchArchiver createParseArchiveStreams', () => { | |||
] as [Readable, ...Writable[]]); | |||
throw new Error('should have failed'); | |||
} catch (err) { | |||
expect(err.message).to.contain('Unexpected number'); | |||
const { message } = err as Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know why you did this, I just hate that there isnt a nicer way of handling this :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed. For actual error handling, I'd definitely go with a type guard, but for tests, the assertion seems a little cleaner (and preferable to a @ts-ignore
, in my opinion), but I could easily be swayed to some other preference.
@@ -38,11 +38,9 @@ const SCRIPT = resolve(REPO_ROOT, 'scripts/functional_test_runner.js'); | |||
const BASIC_CONFIG = require.resolve('../fixtures/simple_project/config.js'); | |||
|
|||
describe('basic config file with a single app and test', function () { | |||
this.timeout(60 * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the timeouts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that Jest handles timeouts as config values, not something that's specified in code for a specific suite the way mocha does. But let me know if there's an equivalent jest syntax for this that I missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see functional test runner and I think this is selenium right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually a mocha test within the functional test runner - my understanding is that it's purpose is to validate that the functional test runner itself can start up correctly. So now this test will be run by jest instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -39,11 +39,9 @@ const SCRIPT = resolve(REPO_ROOT, 'scripts/functional_test_runner.js'); | |||
const FAILURE_HOOKS_CONFIG = require.resolve('../fixtures/failure_hooks/config.js'); | |||
|
|||
describe('failure hooks', function () { | |||
this.timeout(60 * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional test error.
This requires a deeper dive - my expectation here was actually for coverage to increase, as all existing mocha test cases should now be ran by the jest test runner (and I wrote one small new test suite). It may be due to coverage calculation differences between the two runners, but I'll need to investigate the specific cases where it thinks coverage has dropped. |
95f123e
to
ec05c90
Compare
Yes, I think this is because of the coverage runner calculation discrepancies. Once mocha is removed entirely we should re-evaluate. |
ec05c90
to
111a61f
Compare
Verified that 410 of previous 411 mocha test cases are correctly ran by jest. From From this branch/PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
Checking it out now. |
@@ -1,32 +0,0 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come?
I would expect the declaration file if any one utilizes this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted ./index.js
of this package to typescript (now at ./src/index.ts
), and updated ./tsconfig.json
to compile the new typescript files to js. So now the declaration file is at ./target/index.d.ts
, which appears to be the preferred organization for these ts packages
(note that the default export became a named export, which is why I updated the imports; /target
dirs are gitignored)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. didn't catch the switch to ts yes then it should be auto compiled which preferred. thanks!
* under the License. | ||
*/ | ||
|
||
export function unset(object: object, rawPath: string): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. How come?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted all the js files in this directory to typescript. I didn't see any usages that require any of these modules to be compiled to js - it seemed that native ts modules were fine in his case, but are there external packages that need this set up as a js module?
Can you squash your commits and include this type of information in the commit message along with any other helpful contextual information? |
I'm happy to, and and I think that makes sense when it comes to something like compiling release notes. But I purposely updated each package/plugin/module in separately, so the individual commits are actually more compact and clean (I probably should have called that out for reviewers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Ran locally. If rebasing and squashing prior functional test errors will be due to chromedriver issue. Commit message can be modified while squashing and merging.
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Signed-off-by: Josh Romero <rmerqg@amazon.com>
baf3fcc
to
1714f0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks Josh!
Approved. I see the label 2.1. Did we want to backport? Although I think this is okay to backport we should be weary to backport the removal of mocha. |
Agreed, I think we'll have to mark the removal of mocha (#1572) for 3.0. |
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in #1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves #215 Signed-off-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 0e41477)
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in #1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves #215 Signed-off-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 0e41477)
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves opensearch-project#215 Signed-off-by: Josh Romero <rmerqg@amazon.com>
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves opensearch-project#215 Signed-off-by: Josh Romero <rmerqg@amazon.com>
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves opensearch-project#215 Signed-off-by: Josh Romero <rmerqg@amazon.com>
…arch-project#1678) - `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves opensearch-project#215 Signed-off-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 0e41477)
…arch-project#1678) - `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves opensearch-project#215 Signed-off-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 0e41477)
- `src/dev` - migrate mocha tests to jest - `src/test_utils` - migrate mocha tests to jest - `src/dev/license_checker` - migrate mocha tests to jest - `src/legacy/utils` - migrate mocha tests to jest - convert to typescript - add unit test for `version.ts` - `src/plugins/console/server` - migrate mocha tests to jest - convert to typescript - fix type errors - improve test cases - update URL parsing - `packages/opensearch-datemath - migrate mocha tests to jest - convert to typescript - `packages/osd-eslint-plugin-eslint` - migrate mocha tests to jest - rename test file directory - `packages/osd-opensearch-archiver` - migrate mocha tests to jest - rename stub directory - `packages/osd-test` - migrate mocha tests to jest - remove mocha timeouts (not needed in jest) - linting fixes - `packages/osd-test-subject-selector` - migrate mocha tests to jest - convert to typescript - update tsconfig to transpile ts - switch to named exports/imports Verified that 410 of previous 411 mocha test cases are correctly ran by jest. Mocha: 1 passing test (mocha report generation, which will be removed in opensearch-project#1572) Jest: 11155 passing tests (11155 = 10741 previous jest tests + 4 newly added jest test + 410 migrated mocha tests) Resolves opensearch-project#215 Signed-off-by: Josh Romero <rmerqg@amazon.com>
Description
Moves/renames all mocha tests so they can be ran by jest instead. Also some typescript conversion and cleanup.
Issues Resolved
fixes #215
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr