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

feat(testing): allow omitting httpAdapter arg on createNestApplication #9648

Merged
merged 3 commits into from
Jun 24, 2022

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented May 23, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

When using createNestApplication from Test.createTestingModule(), the only way to pass options while still using the default http adapter is by passing undefined (or any falsy value) as the first arg like this:

moduleFixture = await Test.createTestingModule({}).compile()
app = moduleFixture.createNestApplication(undefined, { bodyParser: false })

but to me this interface doesn't feels good from DX POV. See nestjs/docs.nestjs.com#2119 (which can be closed if we merge this PR)

What is the new behavior?

the options must be supplied in the first arg if we want to use the default http adatper, like so:

app = moduleFixture.createNestApplication({ bodyParser: false }) // THIS IS NEW

// Same as:
app = moduleFixture.createNestApplication(new ExpressAdapter(), { bodyParser: false })

which I believe is more intuitve than passing undefined. The old version isn't allowed anymore

Does this PR introduce a breaking change?

  • Yes
  • No

@micalevisk micalevisk force-pushed the feat/testing-module-options branch from 8b00f1b to a4e99bc Compare May 23, 2022 11:13
@coveralls
Copy link

coveralls commented May 23, 2022

Pull Request Test Coverage Report for Build e991088d-6a88-4808-a8e2-6d3ba41d11d0

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build b014cf54-5050-41eb-bd7a-750721daafe5: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@micalevisk micalevisk force-pushed the feat/testing-module-options branch 4 times, most recently from c92712b to d377fd7 Compare May 23, 2022 11:56
@micalevisk micalevisk force-pushed the feat/testing-module-options branch 2 times, most recently from 7bc5c47 to 42d655a Compare May 23, 2022 16:24
@micalevisk
Copy link
Member Author

micalevisk commented May 23, 2022

I notice that the following was not using the fastify adapter

app = module.createNestApplication<NestFastifyApplication>();

After using it, the integration tests starts failing with Uncaught TypeError: Cannot read properties of undefined (reading 'length') 🤔 Let's track this here: #9652

@micalevisk micalevisk force-pushed the feat/testing-module-options branch from 42d655a to d377fd7 Compare May 24, 2022 04:14
@Tony133
Copy link
Contributor

Tony133 commented May 24, 2022

@micalevisk it has been fixed in this PR #9653, so the error as you said does not refer to your PR you made

@micalevisk
Copy link
Member Author

this PR is done but I'll mark it as a draft until those changes made on cors/e2e/fastify.spec.ts and raw-body/e2e/fastify.spec.ts goes to the branch 9.0.0

@micalevisk micalevisk marked this pull request as draft May 24, 2022 14:28
@micalevisk micalevisk changed the title feat(testing)!: allow omitting httpAdapter arg on createNestApplication feat(testing): allow omitting httpAdapter arg on createNestApplication May 26, 2022
@kamilmysliwiec kamilmysliwiec added this to the 9.0.0 milestone May 31, 2022
@micalevisk micalevisk force-pushed the feat/testing-module-options branch from d377fd7 to 721ea53 Compare June 2, 2022 21:19
@kamilmysliwiec kamilmysliwiec mentioned this pull request Jun 15, 2022
12 tasks
@kamilmysliwiec
Copy link
Member

I just merged the latest changes from master to 9.0.0. Not sure if that unblocks you @micalevisk(?)

@micalevisk
Copy link
Member Author

the file cors/e2e/fastify.spec.ts is not ready yet. If I fix below to use fastify instead the test suite will fail

app = module.createNestApplication<NestFastifyApplication>();

@kamilmysliwiec
Copy link
Member

@micalevisk would you like to look into how we can fix it? if you can't find a solution, can always add .skip there - this test doesn't work as expected either way

@micalevisk
Copy link
Member Author

micalevisk commented Jun 17, 2022

Using the latest commit on 9.0.0 (f41ee3d) as the base of this branch, we got an error due to version mismatch (I guess), which is not the one I expected 🤔

image

btw I didn't manage to make the integration tests run locally.

@micalevisk micalevisk marked this pull request as ready for review June 17, 2022 19:26
@kamilmysliwiec
Copy link
Member

@micalevisk updating @fastify/cors to v8 should fix this issue

@kamilmysliwiec
Copy link
Member

Tests are failing now @micalevisk

@micalevisk
Copy link
Member Author

using the latest version of the branch 9.0.0, and after upgrading @fastify/cors to v8, we got this:

image

Also, locally, even using v7 and rm -rf node_modules && npm ci, I got this:

image


fell free to copy the changes I made in this branch if you manage to fix that. Idk what to do by now

@Tony133
Copy link
Contributor

Tony133 commented Jun 24, 2022

@micalevisk i had also received a similar error during some PRs (for example, see #9768), but had no idea how to fix it.

I'm still not sure at the moment, still exploring a little, but I think this problem you had opened is the starting point to solve it #9629.

@kamilmysliwiec kamilmysliwiec merged commit 8293aac into nestjs:9.0.0 Jun 24, 2022
@micalevisk micalevisk deleted the feat/testing-module-options branch June 24, 2022 12:28
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.

4 participants