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

[legacy-framework] Fix typescript jest issue in forgotPassword.test.ts #2110

Merged
merged 10 commits into from
Mar 17, 2021

Conversation

roshan-sama
Copy link
Collaborator

@roshan-sama roshan-sama commented Mar 13, 2021

Closes: blitz-js/legacy-framework#188

What are the changes and their implications?

Modified ...jest.requireActual("blitz")!,to ...jest.requireActual("blitz")!, to prevent typescript compiler from throwing error.

Using the for now, I tried to get the type of that object using a console log but it said "object" as well, but if there's a better type to assign here sometime in the future, we can swap object out with that instead.
console.log(typeof jest.requireActual("@blitzjs/core/server")!)
Console
console.log
object
at app/auth/mutations/forgotPassword.test.ts:12:11

In addition, I also updated the auth examples tests files to be the same as the templates test files

Checklist

  • Changes covered by tests (tests added if needed)
  • PR submitted to blitzjs.com for any user facing changes

Comment on lines 11 to 12
jest.mock("@blitzjs/core/server", () => ({
...jest.requireActual("@blitzjs/core/server")!,
jest.mock("blitz", () => ({
...jest.requireActual<object>("blitz")!,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @roesh!

You have to actually revert this change as it breaks example tests here in the monorepo. Maybe a way to fix it, but needs to be @blitzjs/core/server for now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, got it. Let me revert that

Comment on lines 1 to 25
import {render} from "test/utils"
import { render } from "test/utils"

import Home from "./index"
import { useCurrentUser } from "app/core/hooks/useCurrentUser"

jest.mock("@blitzjs/core", () => ({
...jest.requireActual("@blitzjs/core")!,
useQuery: () => [
{
id: 1,
name: "User",
email: "[email protected]",
role: "user",
},
],
}))
jest.mock("app/core/hooks/useCurrentUser")
const mockUseCurrentUser = useCurrentUser as jest.MockedFunction<typeof useCurrentUser>

test("renders blitz documentation link", () => {
// This is an example of how to ensure a specific item is in the document
// But it's disabled by default (by test.skip) so the test doesn't fail
// when you remove the the default content from the page

// This is an example on how to mock api hooks when testing
mockUseCurrentUser.mockReturnValue({
id: 1,
name: "User",
email: "[email protected]",
role: "user",
})

const {getByText} = render(<Home />)
const element = getByText(/powered by blitz/i)
// @ts-ignore
expect(element).toBeInTheDocument()
const { getByText } = render(<Home />)
const linkElement = getByText(/Documentation/i)
expect(linkElement).toBeInTheDocument()
})
Copy link
Member

Choose a reason for hiding this comment

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

So some change in this file is still causing tests to fail.

Not sure if jest.mock needs to be before the import?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I see. Should the test syncing be a separate PR then? I can commit just the single change to satisfy typescript and we can update the auth tests later?
Also I'm actually not sure how to trigger the CI/CD tests, is there a way to retrigger them and check if the changes I made are OK in the monorepo?

Copy link
Member

Choose a reason for hiding this comment

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

it's up to you. If we get tests passing, then fine to keep the change here.

CI auto runs on each git push, but you can run these failing tests locally by running yarn jest inside examples/auth

Copy link
Collaborator Author

@roshan-sama roshan-sama Mar 17, 2021

Choose a reason for hiding this comment

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

Ah, I see. I reverted back to the original tests, with the only change being the <object> before their jest.requireActual reference. I tried to troubleshoot the index.test.tsx file but I'm actually ran into issues with the forgotpassword mutation as well, so I reverted that back to its original version too. I made sure to run yarn jest this time and all tests passed.
EDIT: Lint failed, I have to make sure I run that too, let me to that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, that was on me, I forgot to push the changes I'd made after resolving a merge conflict, I reran the yarn jest command and eslint --fix and lint, and those passed and waiting for the results of the CI tests

@flybayer
Copy link
Member

Thank you!!

@flybayer flybayer changed the title Fix typescript jest issue and sync test files in auth example Fix typescript jest issue in forgotPassword.test.ts Mar 17, 2021
@flybayer flybayer merged commit 12f5339 into blitz-js:canary Mar 17, 2021
@blitzjs-bot
Copy link
Contributor

Added @roesh contributions for test

@itsdillon itsdillon changed the title Fix typescript jest issue in forgotPassword.test.ts [legacy-framework] Fix typescript jest issue in forgotPassword.test.ts Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript compiler flags error in generated forgotPassword.test.ts file
3 participants