-
Notifications
You must be signed in to change notification settings - Fork 351
provide failing e2e test for defaults issue #8854
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an e2e test covering an empty movies result via GraphQL over gRPC and updates the gRPC subgraph configuration to use a structured source with load.defaults enabled for proto loading. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant C as GraphQL Executor
participant G as gRPC Handler
participant S as Movies gRPC Service
T->>C: Compose schema and start executor
T->>C: Execute GetMovies(genre=HORROR, year=2015)
C->>G: Resolve via gRPC handler
note right of G: Proto loaded with defaults: true
G->>S: gRPC request: GetMovies
S-->>G: Empty dataset response
G-->>C: Mapped response
C-->>T: GraphQL result (empty list)
T->>T: Snapshot assertion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/grpc-example/__snapshots__/grpc-example.test.ts.snap
is excluded by!**/*.snap
📒 Files selected for processing (2)
e2e/grpc-example/grpc-example.test.ts
(1 hunks)e2e/grpc-example/mesh.config.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/grpc-example/grpc-example.test.ts (1)
e2e/utils/tenv.ts (2)
compose
(303-358)serve
(240-302)
🔇 Additional comments (1)
e2e/grpc-example/mesh.config.ts (1)
20-25
: Configuration correctly enables protobuf defaults—ensureYamlConfig.GrpcHandler.source
supports a structured object withfile
andload.defaults
.
it('gets empty movies correctly', async () => { | ||
const { output } = await compose({ services: [movies], output: 'graphql' }); | ||
const { execute } = await serve({ supergraph: output }); | ||
// Genre HORROR does not exist in mock data | ||
const query = /* GraphQL */ ` | ||
query GetMovies { | ||
exampleGetMovies(input: { movie: { genre: HORROR, year: 2015 } }) { | ||
result { | ||
name | ||
year | ||
rating | ||
cast | ||
time { | ||
seconds | ||
} | ||
} | ||
} | ||
} | ||
`; | ||
await expect(execute({ query })).resolves.toMatchSnapshot( | ||
'get-empty-movies-grpc-example-result', | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test scenario may not effectively demonstrate the defaults issue.
The test queries with genre: HORROR
, which doesn't exist in the mock data, and expects an empty result. However, issue #5277 describes a bug where fields within returned objects have null
values instead of their protobuf default values (e.g., an enum defaulting to ENUM_ZERO
). An empty result set won't demonstrate this issue.
To properly test the defaults behavior, consider:
- Query data that exists in the mock (e.g., use
genre: DRAMA
like in the next test) - Include fields that have protobuf defaults (especially enum fields)
- Assert that those fields are populated with their default values, not
null
This would create a test that fails in a way that clearly demonstrates the missing defaults functionality described in the issue.
Example revision:
- it('gets empty movies correctly', async () => {
+ it('populates protobuf default values correctly', async () => {
const { output } = await compose({ services: [movies], output: 'graphql' });
const { execute } = await serve({ supergraph: output });
- // Genre HORROR does not exist in mock data
+ // Query existing data to check if fields with defaults are populated
const query = /* GraphQL */ `
query GetMovies {
- exampleGetMovies(input: { movie: { genre: HORROR, year: 2015 } }) {
+ exampleGetMovies(input: { movie: { genre: DRAMA, year: 2015 } }) {
result {
name
year
rating
cast
+ genre
time {
seconds
}
}
}
}
`;
await expect(execute({ query })).resolves.toMatchSnapshot(
- 'get-empty-movies-grpc-example-result',
+ 'get-movies-with-defaults-result',
);
});
This way, when the test snapshot is generated, you can verify whether enum fields (like genre
) are set to their protobuf default values or null
, directly demonstrating the issue.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
it('gets empty movies correctly', async () => { | |
const { output } = await compose({ services: [movies], output: 'graphql' }); | |
const { execute } = await serve({ supergraph: output }); | |
// Genre HORROR does not exist in mock data | |
const query = /* GraphQL */ ` | |
query GetMovies { | |
exampleGetMovies(input: { movie: { genre: HORROR, year: 2015 } }) { | |
result { | |
name | |
year | |
rating | |
cast | |
time { | |
seconds | |
} | |
} | |
} | |
} | |
`; | |
await expect(execute({ query })).resolves.toMatchSnapshot( | |
'get-empty-movies-grpc-example-result', | |
); | |
}); | |
it('populates protobuf default values correctly', async () => { | |
const { output } = await compose({ services: [movies], output: 'graphql' }); | |
const { execute } = await serve({ supergraph: output }); | |
// Query existing data to check if fields with defaults are populated | |
const query = /* GraphQL */ ` | |
query GetMovies { | |
exampleGetMovies(input: { movie: { genre: DRAMA, year: 2015 } }) { | |
result { | |
name | |
year | |
rating | |
cast | |
genre | |
time { | |
seconds | |
} | |
} | |
} | |
} | |
`; | |
await expect(execute({ query })).resolves.toMatchSnapshot( | |
'get-movies-with-defaults-result', | |
); | |
}); |
🤖 Prompt for AI Agents
In e2e/grpc-example/grpc-example.test.ts around lines 18 to 40, the test queries
for a non-existent genre (HORROR) which returns an empty result and therefore
cannot reveal the protobuf-defaults bug; change the test to query existing mock
data (e.g., use genre: DRAMA as in the next test), include fields that have
protobuf defaults (especially enum fields and nested scalar fields), and assert
(or snapshot) that those fields are populated with their protobuf default values
instead of null so the test fails when defaults are missing.
Description
Added a new test that should currently fail due to
defaults: true
not working when using the grpc loader.As mentioned here I did not quickly manage to run e2e tests without other errors, so I hope I got it right.
Fixes #5277