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

test: update setup, cover useForm's isDirty #116

Merged
merged 17 commits into from
Feb 6, 2024

Conversation

owenconti
Copy link
Contributor

@owenconti owenconti commented Feb 2, 2024

This PR introduces a new test class for testing the form composable, specifically just the isDirty ref for now.

While trying to add this test class, I ran into a few errors with how the tests have been configured. The root of the problem lies within the happy-dom package, issue here: capricorn86/happy-dom#1180. Essentially, happy-dom does not implement the Request/Response classes like the browser does. msw relies on the browser's implementation when mocking requests.

To get around this issue for now, I've downgraded vitest to 0.34.6. However, this only fixed the issue locally for me. When running in CI, I was getting the same error. It turns out that in addition to downgrading vitest, I also had to update Node to v18 from v16. Hopefully that isn't a big deal.

I've gone through the diff and left comments on the individual pieces with reasons for each change.

Copy link

netlify bot commented Feb 2, 2024

Deploy Preview for hybridly failed.

Name Link
🔨 Latest commit 03a9623
🔍 Latest deploy log https://app.netlify.com/sites/hybridly/deploys/65c25435e7511a00088bc34b

beforeEach(() => {
fakeRouterContext()
beforeEach(async() => {
await fakeRouterContext()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensuring we wait for the context to be set up before starting the tests

import { fakeRouterContext, fakePayload, mockSuccessfulUrl } from '../utils'
import { createServer } from '../server'

const server = createServer()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the setup.ts file that was used to set up the server in all of the tests. I was running into some sort of async state issue when running the entire test suite, which now has multiple calls to the mock server.

This fix of creating the server per test file seemed to fix the issue, and should also mean we don't create the server for tests that won't use it.

beforeEach(() => {
fakeRouterContext()
vi.stubGlobal('console', {
warn: vi.fn(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These mocked functions were hiding an error that was being thrown during the request to the mocked server that said that there was no handler defined for the given request.

To define the handlers, you must either pass them in when calling setupServer, or you can call resetHandlers() and pass them in to an existing server.

@@ -27,8 +26,7 @@ export function makeRouterContextOptions(options: PartialDeep<RouterContextOptio
payload: fakePayload(),
adapter: {
resolveComponent: noop,
swapDialog: noop,
swapView: noop,
onViewSwap: noop,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

swapDialog and swapView no longer exist, replaced with onViewSwap

packages/core/test/utils.ts Outdated Show resolved Hide resolved
@owenconti
Copy link
Contributor Author

So I'm running into an issue running the tests locally. Only on the first run after installing the dependencies, the tests fail. If I immediately re-run the tests, then they pass.

The issue is that the router import from @hybridly/core in form.ts returns undefined on the first run. As far as I can tell, vite is supposed to use the alias and find @hybridly/core via the relative path.

@innocenzi innocenzi changed the title Fix tests setup, add initial tests for isDirty value test: update setup, cover useForm's isDirty Feb 3, 2024
packages/core/test/setup.ts Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
export { createRouter, router } from './router'
export type { Router, HybridRequestOptions, NavigationResponse, HybridPayload, ResolveComponent, Method, Progress } from './router'
export { createRouter, router } from './router/router'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixed an issue I was having where the tests would fail on the first run after installing the dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting, do you know why this specifically caused the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I had found some Google results about needing to import * as x but that only applies to default exports, could be a phantom fix.

@innocenzi innocenzi merged commit 8053dae into hybridly:0.x Feb 6, 2024
3 of 7 checks passed
@innocenzi
Copy link
Member

That's awesome. Thanks a lot Owen!

@owenconti owenconti deleted the use-form-tests branch February 6, 2024 17:07
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