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(aws): add missing base path to data routes #672

Merged

Conversation

alebelcor
Copy link
Contributor

Related #670

I was trying to figure out how to add (and run) a unit test in matcher.test.ts but then I saw this issue that mentions all unit tests are broken (?).

If e2e test(s) are required I'd appreciate any help as I'm not very familiar. And let me know if you need something else.

Thanks!

Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: 57f4814

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@opennextjs/aws Patch
app-pages-router Patch
app-router Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sommeeeer
Copy link
Contributor

sommeeeer commented Dec 11, 2024

Unit tests should work again. I guess you could have one inside here:

Copy link
Contributor

@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.

No unit test run just fine.
In packages/tests-unit just run pnpm run dev.
You'll have to add a test here

and modify NextConfig as in here
NextConfig.skipTrailingSlashRedirect = true;

Copy link

pkg-pr-new bot commented Dec 11, 2024

Open in Stackblitz

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

commit: 57f4814

@alebelcor
Copy link
Contributor Author

Added two tests. Test suite seems to run successfully.

Let me know if those are redundant, or you'd rather me add the base path to existing tests instead. I'm happy to update it as you see fit.

@alebelcor alebelcor requested a review from conico974 December 11, 2024 20:42
@alebelcor alebelcor requested a review from sommeeeer December 11, 2024 21:01
Copy link
Contributor

@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.

@sommeeeer
Copy link
Contributor

Looks like it failed on write permission reporting the test coverage in actions/test

@alebelcor
Copy link
Contributor Author

alebelcor commented Dec 11, 2024

So weird, a test started failing locally for some reason now.

@conico974
Copy link
Contributor

@alebelcor I think the reset are done on each describe, not on each test inside describe

@sommeeeer
Copy link
Contributor

sommeeeer commented Dec 11, 2024

if you add this under line 471 it seems to work here. might be that the global beforeEach() will only reset on each describe, but if its inside a describe it will make it reset on each it:

beforeEach(()=> {
  NextConfig.basePath = undefined;
})

Copy link
Contributor

@sommeeeer sommeeeer 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!

Copy link
Contributor

@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 1ece6b4 into opennextjs:main Dec 11, 2024
3 checks passed
@alebelcor alebelcor deleted the fix-missing-base-path-in-data-routes branch December 11, 2024 21:58
@github-actions github-actions bot mentioned this pull request Dec 11, 2024
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.

3 participants