Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

[Testing] Update existing visual/data report helper test suite #157

Merged
merged 4 commits into from
Oct 28, 2020

Conversation

zhongnansu
Copy link
Member

Issue #, if available:

Description of changes:

Due to the previous code changes, need update existing test suites accordingly.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -9,12 +9,14 @@
"module": "commonjs",
"outDir": "./target",
"noEmit": true,
"types": ["node", "jest", "react"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these not needed? I thought this line had to be included

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleting this line will get rid of the type error on jest related types, such as describe(), test, jest.fn(). I am not sure about other negative affects, any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do tests pass when you run yarn test --coverage? Aside from the ones that haven't been updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there are existing snapshot tests that are not fixed yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok- should be fine then. It'll be an easy one-line fix if it does end up breaking anything, anyway 👍

Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

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

LGTM - just one minor comment

@zhongnansu
Copy link
Member Author

Added one more line in visualReportHelper.ts to pass yarn test --coverage , according to istanbuljs/nyc#514

@zhongnansu zhongnansu merged commit 92448da into opendistro-for-elasticsearch:dev Oct 28, 2020
@zhongnansu zhongnansu deleted the testing branch November 10, 2020 22:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants