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.

Sign up for GitHub

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

Jump to bottom

@daml/react: Add an initial test suite #4612

Merged
merged 3 commits into from
Feb 20, 2020
Merged

@daml/react: Add an initial test suite #4612

merged 3 commits into from
Feb 20, 2020

Conversation

hurryabit
Copy link
Contributor

@hurryabit hurryabit commented Feb 19, 2020 •
edited
Loading

This only tests that the useQuery hook does the right thing once the
call to Ledger.query has resolved.

CHANGELOG_BEGIN
CHANGELOG_END

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

This change is Reviewable

@hurryabit hurryabit requested review from a user and aherrmann-da February 19, 2020 19:18
@hurryabit hurryabit force-pushed the daml-react-initial-tests branch from 34424e6 to 914c917 Compare February 19, 2020 19:18
@hurryabit
Copy link
Contributor Author

hurryabit commented Feb 19, 2020 •
edited
Loading

@aherrmann This is the test which success with yarn test (once you've called yarn install in /language-support/ts/ and yarn build @daml/types and @daml/ledger) but fails with

bazel test //language-support/ts/daml-react:test

How the test fails has slightly shifted since we last talked however.

@hurryabit hurryabit force-pushed the daml-react-initial-tests branch 3 times, most recently from 366462b to 577b1fc Compare February 19, 2020 21:32
@aherrmann-da
Copy link
Contributor

The test failure might be caused by the same issue as described in bazel-contrib/rules_nodejs#1383. I'll see if I can apply the same solution as described here to the jest tests.

While investigating the issue I found that there's an eslint plugin for react hooks. That might address some of this issue. See b4f80e6.

@hurryabit
Copy link
Contributor Author

@aherrmann What you're doing in b4f80e6 won't work since it triggers a call to the JSON API on each rerendering rather than only when the query/key has actually changed. The problem is that a new closure for the factory for the query/key is allocated on each rerendering. Thus, the factories must not appear in the dependencies.

Unfortunately, the ESLint rules for hooks won't help us with this special setup hence.

hurryabit and others added 3 commits February 20, 2020 18:20
@hurryabit
@daml/react: Add an initial test suite
a4c3c96 
This only tests that the `useQuery` hook does the right thing once the
call to `Ledger.query` has resolved.

CHANGELOG_BEGIN
CHANGELOG_END
@aherrmann @hurryabit
Workaround for "Invalid hook call."
01284d3
@hurryabit
Fix lint in BUILD file
eb3d419 
CHANGELOG_BEGIN
CHANGELOG_END
@hurryabit hurryabit force-pushed the daml-react-initial-tests branch from d715a35 to eb3d419 Compare February 20, 2020 17:25
ghost
ghost approved these changes Feb 20, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Cool!

@mergify mergify bot merged commit 24cfd1e into master Feb 20, 2020
@mergify mergify bot deleted the daml-react-initial-tests branch February 20, 2020 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aherrmann-da aherrmann-da Awaiting requested review from aherrmann-da

@shayne-fletcher shayne-fletcher Awaiting requested review from shayne-fletcher

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hurryabit @aherrmann-da @aherrmann