Uh oh!
There was an error while loading. Please reload this page.
tests(e2e): Refactor nestjs e2e applications into multiple smaller test applications - #12948
Merged
Conversation
| import * as Sentry from '@sentry/nestjs'; | ||
| import { AppModule } from './app.module'; | ||
| const app1Port = 3030; |
Member
There was a problem hiding this comment.
Suggested change
| constapp1Port=3030; | |
| constPORT=3030; |
| const { httpAdapter } = app.get(HttpAdapterHost); | ||
| Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter)); | ||
| await app.listen(app1Port); |
Member
There was a problem hiding this comment.
Suggested change
| awaitapp.listen(app1Port); | |
| awaitapp.listen(PORT); |
Comment on lines
+49
to
+64
| @Injectable() | ||
| export class AppService2 { | ||
| externalAllowed(headers: Record<string, string>) { | ||
| return { | ||
| headers, | ||
| route: 'external-allowed', | ||
| }; | ||
| } | ||
| externalDisallowed(headers: Record<string, string>) { | ||
| return { | ||
| headers, | ||
| route: 'external-disallowed', | ||
| }; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
m: would generally move each service into its own file and use more descriptive names instead of AppService1 AppService2 etc.
Lms24
reviewed
Jul 18, 2024
Lms24
left a comment
Member
There was a problem hiding this comment.
1st pass review: Would you mind adding the purpose of the three new apps to the PR description? Just a brief description of what they do or how they differ from each other would be great for future and external readers :)
| import * as Sentry from '@sentry/nestjs'; | ||
| import { AppModule } from './app.module'; | ||
| const app1Port = 3030; |
Member
There was a problem hiding this comment.
Suggested change
| constapp1Port=3030; | |
| constPORT=3030; |
| const { httpAdapter } = app1.get(HttpAdapterHost); | ||
| Sentry.setupNestErrorHandler(app1, new BaseExceptionFilter(httpAdapter)); | ||
| await app1.listen(app1Port); |
Member
There was a problem hiding this comment.
Suggested change
| awaitapp1.listen(app1Port); | |
| awaitapp1.listen(PORT); |
chargome
approved these changes
Jul 18, 2024
chargome
left a comment
Member
There was a problem hiding this comment.
Looks good! I would just try to prevent naming services etc. with numerical suffixes.
nicohrubec
commented
Jul 18, 2024
MemberAuthor
@Lms24 Sure good idea, updated the PR description :) |
Lms24
approved these changes
Jul 18, 2024
Comment on lines
+2
to
+3
| "compilerOptions": { | ||
| "module": "commonjs", |
Member
There was a problem hiding this comment.
out of scope for this PR but idea for once #12920 is ready: In one of these apps we could set "moduleResolution": "Node16" to ensure our "workaround" from yesterday works in both, default and sub-path-export-compatible TS configs.
nicohrubec
commented
Jul 18, 2024
MemberAuthor
@chargome I just copied what was there before, but definitely agree about the naming. I split up the two services into separate files and renamed them to TraceInitiator and TraceReceiver, should be more clear |
Contributor
size-limit report 📦
|
nicohrubec added a commit
that referenced
this pull request
Jul 23, 2024
- Adds a new nest root module that can be used to setup the Nest SDK as a replacement for the existing setup (with a function). Instead of calling `setupNestErrorHandler` in the main.ts file, users can now add `SentryModule.forRoot()` (feedback about the name is definitely welcome) as an import in their main app module. This approach is much more native to nest than what we used so far. This root module is introduced in the setup.ts file. - This root module is exported with a submodule export `@sentry/nestjs/setup`, because the SDK now depends on nestjs directly and without this the nest instrumentation does not work anymore, since nest gets imported before Sentry.init gets called, which disables the otel nest instrumentation. - Judging from the e2e tests it seems that this new approach also resolves some issues the previous implementation had, specifically [this issue](#12351) seems to be resolved. The e2e test that was in place, just documented the current (wrong) behavior. So I updated the test to reflect the new (correct) behavior. - I updated all the test applications to use the new approach but kept a copy of the nestjs-basic and nestjs-distributed-tracing with the old setup (now named node-nestjs-basic and node-nestjs-distributed-tracing respectively) so we can still verify that the old setup (which a lot of people use) still keeps working going forward. - Updated/New tests in this PR: - Sends unexpected exception to Sentry if thrown in Submodule - Does not send expected exception to Sentry if thrown in Submodule and caught by a global exception filter - Does not send expected exception to Sentry if thrown in Submodule and caught by a local exception filter - Sends expected exception to Sentry if thrown from submodule registered before Sentry - To accomodate the new tests I added several submodules in the nestjs-with-submodules test-application. These are overall similarly but have important distinctions: - example-module-local-filter: Submodule with a local filter registered using `@UseFilters` on the controller. - example-module-global-filter: Submodule with a global filter registered using APP_FILTER in the submodule definition. - example-module-global-filter-wrong-registration-order: Also has a global filter set with APP_FILTER, but is registered in the root module as first submodule, even before the SentryIntegration is initialized. This case does not work properly in the new setup (Sentry should be set first), so this module is used for tests documenting this behavior. - Also set "moduleResolution": "Node16" in the nestjs-basic sample app to ensure our submodule-export workaround works in both, default and sub-path-export-compatible TS configs as was suggested [here](#12948 (comment)).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor of the existing nestjs test applications. Before we had one sample application testing everything nest-related. This PR splits them up into three applications to make it more readable and easier to understand what is being tested. It also allows for iterating a bit quicker in local development.
No new functionality was added. Will add more tests in a follow-up.
The three new services are: