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

fix: updates helppanel tests and removes others #50

Merged
merged 7 commits into from
May 7, 2021

Conversation

noahamac
Copy link
Contributor

@noahamac noahamac commented May 6, 2021

Thanks for adding that extension context code. I moved it over to a wrapper function we can reuse.

I ran into a whole bunch of trouble trying to get d3 to play nicely with jest and react-testing-library. I chose to start working on because I thought jest would have access to the resultant svg, but that doesn't look like the case.
jsdom/jsdom#300

The answer I was able to find was to test the individual accessors. That doesn't make a lot of sense to me, as it's what Typescript is doing. And given my intention to eliminate d3 control of dom, doesn't seem efficient.

I removed the snapshot tests because they weren't really testing anything as shallow renders.

I can address by typing the svg. This is what I've proposed, and will work towards in medium term. In short term, the most important test targets are:

  • Metadata panel and utils
  • LookmlDiagrammer utils

As well as improving the integration tests and diagram generation tests, which I feel better about.

I want to merge #47 , #49 , and this, check & update integration tests, and then cut a Marketplace release. Will work on the above items in future PRs, blocking net new work.

@noahamac noahamac requested a review from bryans99 May 6, 2021 05:46
@noahamac
Copy link
Contributor Author

noahamac commented May 6, 2021

Blocked by #49

bryans99
bryans99 previously approved these changes May 6, 2021
Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

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

I think we want to gitignore the eslintcache but I've mentioned this on another PR

.eslintcache Outdated Show resolved Hide resolved
Base automatically changed from noahamac/prettier to main May 6, 2021 23:51
@noahamac noahamac merged commit c4c9fa8 into main May 7, 2021
@noahamac noahamac deleted the noahamac/helppanel-test branch May 7, 2021 00:59
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.

2 participants