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

Fix broken tests #551

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Fix broken tests #551

merged 1 commit into from
Oct 15, 2024

Conversation

socsieng
Copy link
Contributor

Initial PR to try and fix broken tests.

For: #539

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Oct 14, 2024 •
edited
Loading

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@551

commit: b7cc2a4

@socsieng socsieng mentioned this pull request Oct 14, 2024
Open
3 tasks
@socsieng socsieng force-pushed the fix/tests branch 6 times, most recently from f109078 to 364561f Compare October 14, 2024 14:46
conico974
conico974 reviewed Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

As for the test failing it's probably some kind of config in vitest itself.
Does it work locally ?

packages/tests-unit/tests/adapter.utils.test.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather have one test file per file. It makes it easier to know what's covered and what's not.
So it would be split in 2 core/routing/util.test.ts and http/util.test.ts

@socsieng
Copy link
Contributor Author

As for the test failing it's probably some kind of config in vitest itself. Does it work locally ?

It was working locally. It is failing now after git clean. Looking into it.

@socsieng socsieng force-pushed the fix/tests branch 2 times, most recently from 7fcef80 to c492543 Compare October 14, 2024 21:24
@socsieng socsieng marked this pull request as ready for review October 14, 2024 21:28
@socsieng
Copy link
Contributor Author

Tests fixed. Let me know if you're happy to proceed. I can continue to flesh out additional tests in this PR or separate pull requests if you prefer.

@socsieng socsieng force-pushed the fix/tests branch 2 times, most recently from 6c2a682 to 367ab3d Compare October 14, 2024 21:43
conico974
conico974 requested changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

Separate PR will be easier to review.
If you could just move the cache.test.ts following the same logic as the other files.
And delete example.test.ts

conico974
conico974 approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks !!!

@conico974 conico974 merged commit e233d8c into opennextjs:main Oct 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conico974 conico974 conico974 approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@socsieng @conico974