chore(performance): Make improved tests for graphql search #2030
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes to enhance GraphQL performance testing. A new test suite path is added in the GitHub Actions workflow to include a random search script. The GraphQL search functions are updated to incorporate a dedicated Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (5)
35-43: Check for duplicated filter combinationsThere appear to be duplicate filter combination definitions with the same labels:
- Line 35 and 41 both define
"search-party-createdafter"- Line 36 and 42 both define
"search-party-createdbefore"This might lead to confusion when analyzing results, as the same label will be used for different test runs.
Consider renaming one set of the duplicated filter combinations to ensure unique labels across all test scenarios:
- {label: "search-party-createdafter", filters: ["search", "party", "createdAfter"]}, - {label: "search-party-createdbefore", filters: ["search", "party", "createdBefore"]}, + {label: "search-party-createdafter-2", filters: ["search", "party", "createdAfter"]}, + {label: "search-party-createdbefore-2", filters: ["search", "party", "createdBefore"]},
79-83: Improve date filter values and default case handlingTwo issues with the filter value generation:
createdAfterandcreatedBeforeuse the same date value (7 days ago), which doesn't create a meaningful time range when both filters are used together.The default case returns a random resource URI, which may not be appropriate for all unhandled filter types.
Consider implementing these improvements:
- case "createdAfter": return new Date(Date.now() - 7*24*60*60*1000).toISOString(); - case "createdBefore": return new Date(Date.now() - 7*24*60*60*1000).toISOString(); + case "createdAfter": return new Date(Date.now() - 14*24*60*60*1000).toISOString(); // 14 days ago + case "createdBefore": return new Date(Date.now() - 1*24*60*60*1000).toISOString(); // 1 day ago case "search": return label.includes("nohit") ? randomItem(texts_no_hit) : randomItem(texts); - default: return "urn:altinn:resource:" +randomItem(resources); + default: + console.warn(`Unhandled filter type: ${filter}`); + return null; // or implement appropriate values for other filter types
90-92: Simplify redundant null checkThe null check for
numberOfEndUsersis redundant since the default parameter already handles undefined values.You can simplify by removing the redundant check:
export function setup(numberOfEndUsers = defaultNumberOfEndUsers) { const tokenOptions = { scopes: "digdir:dialogporten" } - if (numberOfEndUsers === null) { - numberOfEndUsers = defaultNumberOfEndUsers; - } const endusers = getEndUserTokens(numberOfEndUsers, tokenOptions); return endusers }The default parameter will only handle undefined values, not null. If null is a possible input value that needs to be caught, consider this alternative:
export function setup(numberOfEndUsers = defaultNumberOfEndUsers) { const tokenOptions = { scopes: "digdir:dialogporten" } - if (numberOfEndUsers === null) { - numberOfEndUsers = defaultNumberOfEndUsers; - } + numberOfEndUsers = numberOfEndUsers ?? defaultNumberOfEndUsers; const endusers = getEndUserTokens(numberOfEndUsers, tokenOptions); return endusers }
50-56: Consider making setup timeout configurableBased on the retrieved learning, hardcoded values like the setupTimeout will be made configurable via environment variables once Altinn-testtools is fixed.
Consider updating the setupTimeout to use an environment variable with a fallback:
export let options = { - setupTimeout: '10m', + setupTimeout: (__ENV.SETUP_TIMEOUT ?? '10m'), summaryTrendStats: ['avg', 'min', 'med', 'max', 'p(95)', 'p(99)', 'p(99.5)', 'p(99.9)', 'count'], thresholds: { checks: ['rate>=1.0'] } };
116-120: LGTM! Consider adding more detailed response validationThe GraphQL request and basic response validation looks good. For more robust testing, consider adding validation for the expected structure of the GraphQL response data.
You could enhance the validation to check for specific fields in the response:
describe('Perform graphql dialog list', () => { let r = postGQ(getGraphqlParty(searchParams), paramsWithToken); expectStatusFor(r).to.equal(200); expect(r, 'response').to.have.validJsonBody(); + const body = r.json(); + expect(body, 'response body').to.have.property('data'); + expect(body.data, 'response data').to.have.property('party'); + expect(body.data.party, 'party data').to.have.property('dialogElements'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/dispatch-k6-performance.yml(1 hunks)tests/k6/tests/graphql/performance/graphql-search.js(1 hunks)tests/k6/tests/graphql/performance/graphqlRandomSearch.js(1 hunks)tests/k6/tests/performancetest_common/simpleSearch.js(2 hunks)tests/k6/tests/performancetest_data/graphql-search.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (1)
Learnt from: dagfinno
PR: Altinn/dialogporten#1843
File: tests/k6/tests/enduser/performance/enduser-search.js:5-5
Timestamp: 2025-02-12T10:32:18.061Z
Learning: The hardcoded values in k6 performance tests (e.g., defaultNumberOfEndUsers=2799, setupTimeout='10m') are temporary and will be made configurable via environment variables once Altinn-testtools is fixed.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
🔇 Additional comments (7)
.github/workflows/dispatch-k6-performance.yml (1)
58-58: Added new test suite path for GraphQL random search.This addition enables the execution of the new GraphQL random search performance test script through the existing workflow dispatch mechanism.
tests/k6/tests/graphql/performance/graphql-search.js (1)
41-42: Improved search parameter handling.The code now properly structures search parameters as an object with a formatted party identifier, which aligns with the updated signature of the
graphqlSearchfunction. This change enhances the flexibility of search operations and provides a more consistent interface.tests/k6/tests/performancetest_data/graphql-search.js (2)
3-17: Enhanced flexibility for GraphQL query construction.This is a significant improvement to the
getGraphqlPartyfunction. The function now:
- Accepts a generic
inputsobject instead of a single identifier- Dynamically constructs query parameters based on the input object's keys and values
- Intelligently handles different parameter types (list vs. non-list elements)
This change makes the GraphQL query generation more flexible and reusable for various search scenarios.
20-20: Updated query syntax for dynamic search parameters.The search query now properly incorporates the dynamically generated input string into the GraphQL query.
tests/k6/tests/performancetest_common/simpleSearch.js (2)
153-153: Updated function signature to support enhanced search parameters.The
graphqlSearchfunction signature has been modified to accept a dedicatedsearchParamsparameter, which aligns with the changes made to the GraphQL query construction. This improves the function's flexibility and allows for more complex search operations.
171-171: Updated GraphQL query generation to use search parameters object.The function now passes the
searchParamsobject togetGraphqlPartyinstead of just theenduser.ssn, leveraging the enhanced flexibility of the updated query builder.tests/k6/tests/graphql/performance/graphqlRandomSearch.js (1)
8-8: Note about hardcoded valuesThe hardcoded value
defaultNumberOfEndUsers=2799aligns with the retrieved learning that indicates these values are temporary and will be made configurable via environment variables once Altinn-testtools is fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (1)
68-68: Consistency in filter namingNote that this file uses lowercase "search" while other files use uppercase "Search". Consider standardizing filter names across all test files.
- case "search": return label.includes("nohit") ? randomItem(texts_no_hit) : randomItem(texts); + case "search": return label.includes("nohit") ? randomItem(texts_no_hit) : randomItem(texts);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/k6/tests/enduser/performance/enduserRandomSearch.js(1 hunks)tests/k6/tests/graphql/performance/graphqlRandomSearch.js(1 hunks)tests/k6/tests/performancetest_common/readTestdata.js(1 hunks)tests/k6/tests/serviceowner/performance/serviceOwnerRandomSearch.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
tests/k6/tests/enduser/performance/enduserRandomSearch.js (1)
Learnt from: dagfinno
PR: Altinn/dialogporten#1843
File: tests/k6/tests/enduser/performance/enduser-search.js:5-5
Timestamp: 2025-02-12T10:32:18.061Z
Learning: The hardcoded values in k6 performance tests (e.g., defaultNumberOfEndUsers=2799, setupTimeout='10m') are temporary and will be made configurable via environment variables once Altinn-testtools is fixed.
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (1)
Learnt from: dagfinno
PR: Altinn/dialogporten#1843
File: tests/k6/tests/enduser/performance/enduser-search.js:5-5
Timestamp: 2025-02-12T10:32:18.061Z
Learning: The hardcoded values in k6 performance tests (e.g., defaultNumberOfEndUsers=2799, setupTimeout='10m') are temporary and will be made configurable via environment variables once Altinn-testtools is fixed.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
- GitHub Check: build / build-and-test
🔇 Additional comments (11)
tests/k6/tests/performancetest_common/readTestdata.js (1)
101-114: Code refactoring: Good centralization of test dataMoving these test data constants to a common location is a good practice. This improves maintainability by ensuring that all test scripts reference the same data, making updates easier and reducing duplication.
tests/k6/tests/serviceowner/performance/serviceOwnerRandomSearch.js (2)
8-8: Clean import of centralized test dataGood refactoring to use the centralized constants from readTestdata.js rather than locally defined ones.
11-11: Accurate comment updateCorrect update from "enduser filters" to "serviceowner filters" to reflect the proper context of this file.
tests/k6/tests/enduser/performance/enduserRandomSearch.js (1)
7-7: Good refactoring to use centralized constantsAppropriate use of imported constants from the common module, which maintains consistency with other test files.
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (7)
1-8: Well-structured imports for the new GraphQL testThe imports are comprehensive and appropriate for the test's functionality, including the centralized test data constants.
10-10: Consider environment variable for defaultNumberOfEndUsersI see you're using a hardcoded value with a fallback to environment variable. As noted in previous feedback, this is temporary until Altinn-testtools is fixed.
15-35: Comprehensive filter combinations for thorough testingThe test includes an extensive set of filter combinations that cover various search scenarios, including both successful searches and "no hit" cases. This provides good coverage for performance testing.
37-48: Well-configured test options with appropriate thresholdsThe performance test options are well-defined with comprehensive summary statistics and appropriate thresholds for each filter combination.
50-71: Effective parameter generation functionsThe query parameter generation functions are well-designed, using random selection to create varied test scenarios. The filter value function handles different filter types appropriately.
73-82: Well-implemented setup function with appropriate token configurationThe setup function properly initializes the test environment by retrieving end-user tokens with the correct scope.
84-108: Complete test implementation with proper error handlingThe main test function is well-structured, with appropriate error handling and response validation. It selects random users and parameters, ensuring comprehensive test coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (1)
71-80: Redundant null check for numberOfEndUsers parameter.The parameter
numberOfEndUsersalready has a default value in the function signature, so the null check on lines 75-77 is redundant since JavaScript would never pass null unless explicitly specified.Consider removing the redundant null check:
export function setup(numberOfEndUsers = defaultNumberOfEndUsers) { const tokenOptions = { scopes: "digdir:dialogporten" } - if (numberOfEndUsers === null) { - numberOfEndUsers = defaultNumberOfEndUsers; - } const endusers = getEndUserTokens(numberOfEndUsers, tokenOptions); return endusers }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (1)
Learnt from: dagfinno
PR: Altinn/dialogporten#1843
File: tests/k6/tests/enduser/performance/enduser-search.js:5-5
Timestamp: 2025-02-12T10:32:18.061Z
Learning: The hardcoded values in k6 performance tests (e.g., defaultNumberOfEndUsers=2799, setupTimeout='10m') are temporary and will be made configurable via environment variables once Altinn-testtools is fixed.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
- GitHub Check: build / build-and-test
🔇 Additional comments (9)
tests/k6/tests/graphql/performance/graphqlRandomSearch.js (9)
1-7: Well-structured imports provide a solid foundation for the test script.The imports cover all necessary functionalities for the GraphQL performance tests, including utility functions, token management, and test assertions.
9-10: Environment variables configuration aligns with project standards.The code correctly implements environment variable configurations with appropriate defaults. As noted in the retrieved learning, these hardcoded values are temporary and will be made configurable via environment variables once Altinn-testtools is fixed.
15-33: Comprehensive test scenarios with varied filter combinations.The defined filter combinations provide good coverage for testing the GraphQL search functionality with different parameter combinations, including edge cases like "no hit" scenarios.
35-46: Thorough performance metrics configuration.The options setup with detailed summary trend statistics and appropriate thresholds for each filter combination ensures comprehensive performance monitoring.
48-56: Well-designed query parameter generation function.The function effectively generates diverse search parameters based on randomly selected filter combinations.
58-69: Date values for createdAfter and createdBefore are identical.Both
createdAfterandcreatedBeforefilters use the same date (7 days ago), which might not provide meaningful results when both filters are used together in a search query.Consider using different date values for these filters to create more realistic test scenarios:
- case "createdAfter": return new Date(Date.now() - 7*24*60*60*1000).toISOString(); - case "createdBefore": return new Date(Date.now() - 7*24*60*60*1000).toISOString(); + case "createdAfter": return new Date(Date.now() - 14*24*60*60*1000).toISOString(); // 14 days ago + case "createdBefore": return new Date(Date.now()).toISOString(); // Current date
82-95: Well-implemented test execution with clear parameter setup.The main test function correctly selects random test data, sets up request parameters with proper authentication and headers, and includes appropriate tagging for metrics collection.
97-99: Conditional tracing implementation for performance analysis.The code correctly implements conditional tracing based on the environment variable, which allows for selective performance debugging.
101-106: Comprehensive test assertions with proper error handling.The test includes appropriate assertions for HTTP status and response body validation to ensure the GraphQL endpoint functions correctly.
🤖 I have created a release *beep* *boop* --- ## [1.58.0](v1.57.7...v1.58.0) (2025-03-11) ### Features * Add Name to ApiAction as optional field ([#2034](#2034)) ([95ba41e](95ba41e)) ### Bug Fixes * **webapi:** Move swagger server override to correct post processing step ([#2037](#2037)) ([e2ba5d5](e2ba5d5)) ### Miscellaneous Chores * **performance:** Make improved tests for graphql search ([#2030](#2030)) ([3688892](3688892)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Improved test for graphql search, uses combinations of query-options
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)