chore(e2e): Harden e2e tests, add cleanup routine after all-tests run #1314
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request encompasses updates to documentation and test files related to a testing framework for dialogs. Key changes include enhancements to the README documentation for clarity, modifications to constants in configuration files, the introduction of a sentinel check for unpurged dialogs, and updates to various test files to improve validation and cleanup processes. The changes aim to refine the testing framework without altering its core functionalities. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (14)
tests/k6/tests/all-tests.js (2)
9-10: LGTM: sentinelCheck() is correctly implemented.The addition of
sentinelCheck()at the end ofrunAllTests()is well-placed and aligns with the PR objectives. It will help detect and clean up any leftover dialogs from previous test runs.Consider slightly modifying the comment to be more specific:
- // Run sentinel check last, which will warn about and purge any leftover dialogs + // Run sentinel check last to warn about and purge any dialogs left over from previous test runsThis change emphasizes that the leftover dialogs are from previous runs, not the current one.
1-11: Summary: Excellent addition of sentinel check to improve test reliability.The changes in this file effectively implement the sentinel check mechanism as described in the PR objectives. By adding the
sentinelCheck()at the end of therunAllTests()function, you've ensured that any leftover dialogs from previous test runs will be detected and purged. This addition significantly improves the reliability and consistency of the testing process by preventing interference between test runs.To further enhance the robustness of your testing framework, consider implementing the following:
- Add error handling around the
sentinelCheck()call to ensure that any exceptions it throws don't prevent the completion of the test suite.- Consider adding a configurable option to skip the sentinel check in certain scenarios, if needed for debugging or performance reasons.
- Implement logging of the sentinel check results for easier debugging and monitoring of test cleanliness over time.
tests/k6/tests/serviceowner/dialogDetails.js (1)
38-41: LGTM: Cleanup section added successfully.The new cleanup section effectively addresses the PR objective of adding missing cleanup routines. It's correctly placed at the end of the test suite and uses the appropriate status code check for a successful deletion.
Consider adding a comment explaining the significance of the 204 status code for clarity:
describe('Cleanup', () => { let r = purgeSO('dialogs/' + dialogId); // 204 status code indicates successful deletion with no content returned expectStatusFor(r).to.equal(204); });tests/k6/common/sentinel.js (2)
6-10: LGTM with a minor suggestion: Consider using const for immutable variables.The token options and test description are well-defined. The scopes seem comprehensive for the required operations, and the test description clearly states the purpose of this sentinel check.
Consider using
constinstead ofletfortokenOptionsas it doesn't appear to be reassigned:-let dialogId = null; -const tokenOptions = { +const dialogId = null; +const tokenOptions = {
1-30: Consider adding a cleanup step and kudos for using a sentinel value.The overall structure of this test file is good for its single purpose. Here are some final thoughts:
Great job using a sentinel value for identifying test-related dialogs. This practice helps isolate test data and prevent interference with real data.
Consider adding a cleanup step after the test to ensure the test environment is fully reset. This could involve a final check and purge operation.
Here's a suggestion for adding a cleanup step:
// At the end of the exported function describe('Post-test cleanup', () => { it('should ensure all sentinel dialogs are removed', async () => { const r = getSO('dialogs/?Search=' + sentinelValue, null, tokenOptions); expect(r, 'response').to.have.validJsonBody(); const response = r.json(); expect(response.items.length, 'remaining sentinel dialogs').to.equal(0); }); });This additional step will help ensure that the test environment is clean after each run, improving test isolation and reliability.
tests/k6/common/config.js (1)
43-43: LGTM! Consider adding usage documentation.The addition of
sentinelValuealigns with the PR objective of establishing a sentinel mechanism to detect tests that fail to clean up. The constant name and value are descriptive.Consider adding a brief comment explaining how this sentinel value will be used in the testing process. This would improve maintainability and help other developers understand its purpose quickly.
tests/k6/common/dialog.js (2)
44-46: LGTM: Addition of sentinel value to search tags.The addition of the sentinel value aligns with the PR objectives and is implemented correctly. It will help detect tests that fail to clean up, as intended.
Consider adding a brief comment explaining what the sentinel value is used for, to improve code readability:
// Always set the sentinel string that we use to check for leftover dialogs after the run +// This helps identify dialogs that weren't properly cleaned up during testing tags.push({ "value": sentinelValue });
Add sentinelValue import to all test files using setSearchTags
The following test files utilize
setSearchTagsbut are missing thesentinelValueimport:
tests/k6/common/testimports.jstests/k6/tests/enduser/dialogSearch.jstests/k6/tests/serviceowner/dialogSearch.jsPlease import
sentinelValuein these files to ensure consistent test cleanup and detection of leftover dialogs.🔗 Analysis chain
Line range hint
1-46: Summary: Changes align with PR objectives and improve test cleanup detection.The modifications to this file are minimal and focused, aligning well with the PR objectives. The addition of the sentinel value to search tags will help identify dialogs that weren't properly cleaned up during testing.
To ensure consistency across the testing framework:
This will help identify if similar changes are needed in other test-related files for consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if similar sentinel value additions are needed in other test files # Search for files that might need similar changes echo "Files that might need similar sentinel value additions:" fd -e js -e ts --type f . tests | xargs grep -l "setSearchTags" # Check if sentinelValue is imported in these files echo "\nFiles already importing sentinelValue:" fd -e js -e ts --type f . tests | xargs grep -l "import.*sentinelValue"Length of output: 600
tests/k6/README.md (3)
40-41: Excellent additions to test file guidelines!The new emphasis on test file independence and state cleanup is a great improvement to the testing guidelines. These practices will help ensure more reliable and maintainable tests.
Consider a minor wording adjustment for clarity:
-Test files **must** clean up any state they create (see `purgeSO` below), to ensure that the test environment is in a consistent state. A check is implemented in `tests/all-tests.js` to ensure that all dialogs created during the gets purged. Failing to clean up will fail the test run. +Test files **must** clean up any state they create (see `purgeSO` below) to ensure that the test environment remains in a consistent state. A check is implemented in `tests/all-tests.js` to ensure that all dialogs created during the tests are purged. Failing to clean up will cause the test run to fail.Also applies to: 44-45
🧰 Tools
🪛 LanguageTool
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...or with others in any sequential order, ie not rely on state created by tests in o...(AI_HYDRA_LEO_MISSING_COMMA)
97-97: Improve Markdown formatting for the TODO itemThe formatting change improves readability. However, to address the Markdownlint warning about bare URLs, consider wrapping the URL in angle brackets.
Apply this change:
-* Add support for getting real Maskinporten tokens, see https://github.com/mulesoft-labs/js-client-oauth2 +* Add support for getting real Maskinporten tokens, see <https://github.com/mulesoft-labs/js-client-oauth2>🧰 Tools
🪛 Markdownlint
97-97: null
Bare URL used(MD034, no-bare-urls)
40-42: Minor language improvementsConsider the following suggestions to enhance readability and grammar:
- Add a comma after "order" on line 40:
-Test files should be self-contained and handle both being run alone or with others in any sequential order, ie not rely on state created by tests in other files. +Test files should be self-contained and handle both being run alone or with others in any sequential order, i.e., not rely on state created by tests in other files.
- Replace "in order to" with "to" on line 42 for conciseness:
-There are several utility functions provided in order to create tests, that can be imported from `testimports.js` residing the `common` directory. +There are several utility functions provided to create tests, that can be imported from `testimports.js` residing in the `common` directory.
- Add the preposition "in" before "the
commondirectory" on line 42:-There are several utility functions provided in order to create tests, that can be imported from `testimports.js` residing the `common` directory. +There are several utility functions provided in order to create tests, that can be imported from `testimports.js` residing in the `common` directory.🧰 Tools
🪛 LanguageTool
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...or with others in any sequential order, ie not rely on state created by tests in o...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~42-~42: Consider a shorter alternative to avoid wordiness.
Context: ... are several utility functions provided in order to create tests, that can be imported from...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~42-~42: Possible missing preposition found.
Context: ...imported fromtestimports.jsresiding thecommondirectory. Test files *must...(AI_HYDRA_LEO_MISSING_IN)
tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (1)
162-165: Good addition of cleanup step, consider refactoring.The inclusion of a cleanup routine after the 'transmissionOpened' dialog creation test is consistent with the previous test and maintains a clean test environment.
Consider refactoring the cleanup steps into a separate function to reduce code duplication. This could be implemented as follows:
function cleanupDialog(dialogId) { let r = purgeSO('dialogs/' + dialogId); expectStatusFor(r).to.equal(204); } // Usage in tests cleanupDialog(r.json());This refactoring would make the code more DRY and easier to maintain.
tests/k6/tests/serviceowner/dialogSearch.js (1)
167-167: LGTM: Updated filters and improved readabilityAll API calls now correctly use the
defaultFilter, which includes theUpdatedAfterparameter. These changes are consistent with the new filtering mechanism and ensure that all filters use the updated filtering logic. The addition of empty lines (212 and 218) improves code readability.Consider adding a comment above line 212 to explain the purpose of the empty line, such as:
// Separate test cases from cleanup sectionThis would further enhance code readability and maintainability.
Also applies to: 175-175, 183-183, 190-190, 198-198, 207-207, 212-212, 218-218
tests/k6/tests/serviceowner/testdata/01-create-dialog.js (1)
17-18: LGTM: Sentinel value added to searchTags.The addition of the
sentinelValueto thesearchTagsarray is well-implemented and aligns with the PR objective of establishing a sentinel mechanism for detecting unpurged dialogs. The accompanying comment clearly explains its purpose.Consider slightly modifying the comment for clarity:
- { "value": sentinelValue } // Do not remove this, this is used to look for unpurged dialogs after a run + { "value": sentinelValue } // Do not remove this; it is used to detect unpurged dialogs after a test run
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- tests/k6/README.md (2 hunks)
- tests/k6/common/config.js (2 hunks)
- tests/k6/common/dialog.js (2 hunks)
- tests/k6/common/sentinel.js (1 hunks)
- tests/k6/tests/all-tests.js (1 hunks)
- tests/k6/tests/enduser/dialogDetails.js (0 hunks)
- tests/k6/tests/enduser/dialogSearch.js (1 hunks)
- tests/k6/tests/enduser/dialogSystemLabelLog.js (0 hunks)
- tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (3 hunks)
- tests/k6/tests/serviceowner/dialogDetails.js (2 hunks)
- tests/k6/tests/serviceowner/dialogSearch.js (5 hunks)
- tests/k6/tests/serviceowner/testdata/01-create-dialog.js (2 hunks)
💤 Files with no reviewable changes (2)
- tests/k6/tests/enduser/dialogDetails.js
- tests/k6/tests/enduser/dialogSystemLabelLog.js
🧰 Additional context used
🪛 LanguageTool
tests/k6/README.md
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...or with others in any sequential order, ie not rely on state created by tests in o...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~42-~42: Consider a shorter alternative to avoid wordiness.
Context: ... are several utility functions provided in order to create tests, that can be imported from...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~42-~42: Possible missing preposition found.
Context: ...imported fromtestimports.jsresiding thecommondirectory. Test files *must...(AI_HYDRA_LEO_MISSING_IN)
🪛 Markdownlint
tests/k6/README.md
97-97: null
Bare URL used(MD034, no-bare-urls)
🔇 Additional comments (24)
tests/k6/tests/all-tests.js (1)
3-3: LGTM: Import statement for sentinelCheck is correct.The import statement for
sentinelCheckis properly added and aligns with the PR objectives of introducing a sentinel mechanism for detecting tests that fail to clean up.tests/k6/tests/serviceowner/dialogDetails.js (3)
6-7: LGTM: Import statement updated correctly.The addition of
purgeSOto the import statement is consistent with the PR objective of implementing cleanup routines. This change sets the foundation for the new cleanup functionality.
Line range hint
1-41: LGTM: Changes maintain file structure and consistency.The additions to this file are well-integrated with the existing test suite structure. The new cleanup section follows the established pattern of using
describeblocks, and thedialogIdvariable is used consistently throughout the file. These changes enhance the test suite without disrupting the existing structure or coding style.
Line range hint
1-41: Summary: Changes successfully implement cleanup routine.The modifications to this file effectively address the PR objective of adding missing cleanup routines. The new
purgeSOimport and the cleanup section at the end of the test suite are well-implemented and maintain the existing code structure and style. These changes will help ensure that resources are properly managed after test execution, contributing to a more robust testing environment.tests/k6/common/sentinel.js (2)
1-4: LGTM: Imports and function declaration look good.The import statements are appropriate for the file's purpose, bringing in necessary testing utilities and configuration. Exporting the main function as default is a good practice for a single-function module.
11-14: LGTM: API call and response validation are well-implemented.The API call to retrieve dialogs and the subsequent validations are correctly implemented. Good use of
expectStatusForandexpectfor validating the response status and body. The response is properly parsed as JSON for further processing.tests/k6/common/config.js (2)
42-42: LGTM! Improved readability.The addition of a blank line improves the readability of the configuration file by separating different sections.
20-21: LGTM! Verify new default values.The changes to
defaultEndUserOrgNoanddefaultEndUserSsnalign with the PR objective of introducing a new default synthetic person/organization for testing. The updated comments provide good context.Please confirm that the new values are valid Norwegian organization numbers and social security numbers. You can use the following script to check if these values are used consistently across the codebase:
tests/k6/common/dialog.js (1)
2-2: LGTM: Import statement for sentinelValue.The import statement is correctly placed and necessary for the new functionality in the
setSearchTagsfunction.tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (3)
109-112: Excellent addition of cleanup step!The inclusion of a cleanup routine after the dialog creation test is a great practice. It ensures that the test environment remains clean and prevents potential interference between test cases. The check for the correct status code (204) after purging is also commendable.
192-192: Good practice: Newline at end of file.Adding a newline at the end of the file is a good practice. It ensures compatibility with various tools and follows the POSIX standard for text files. This can prevent issues when concatenating files or using certain version control operations.
Line range hint
1-192: Summary of changes and suggestions for improvementThe changes in this file align well with the PR objectives of hardening e2e tests and adding cleanup routines. The addition of cleanup steps after dialog creation tests is a significant improvement that will help maintain a clean test environment and prevent interference between test cases.
To further enhance the code:
- Consider refactoring the cleanup steps into a separate function to reduce code duplication, as suggested earlier.
- Review other test files in the suite to ensure consistent application of cleanup routines.
- Consider adding comments explaining the purpose of each test case and the rationale behind the chosen assertions.
These changes have significantly improved the robustness of the test suite. Great work on enhancing the test quality!
tests/k6/tests/serviceowner/dialogSearch.js (8)
46-47: LGTM: Improved filtering mechanismThe introduction of
updatedAfteranddefaultFilteraligns with the PR objective of usingUpdatedAfterinstead ofChangedAfter. This change helps prevent conflicts with other dialogs by filtering based on the update timestamp.
104-104: LGTM: Updated title search filterThe API call for title search now correctly uses the
defaultFilter, which includes theUpdatedAfterparameter. This change is consistent with the new filtering mechanism and ensures that the title search uses the updated filtering logic.
111-111: LGTM: Updated body search filterThe API call for body search now correctly uses the
defaultFilter, which includes theUpdatedAfterparameter. This change is consistent with the new filtering mechanism and ensures that the body search uses the updated filtering logic.
118-118: LGTM: Updated sender name search filterThe API call for sender name search now correctly uses the
defaultFilter, which includes theUpdatedAfterparameter. This change is consistent with the new filtering mechanism and ensures that the sender name search uses the updated filtering logic.
125-125: LGTM: Updated extended status filterThe API call for extended status filter now correctly uses the
defaultFilter, which includes theUpdatedAfterparameter. This change is consistent with the new filtering mechanism and ensures that the extended status filter uses the updated filtering logic.
132-132: LGTM: Updated list with limit filtersThe API calls for list with limit now correctly use the
defaultFilter, which includes theUpdatedAfterparameter. These changes are consistent with the new filtering mechanism and ensure that the list with limit uses the updated filtering logic.Also applies to: 139-139
150-150: LGTM: Updated list with custom orderBy filtersThe API calls for list with custom orderBy now correctly use the
defaultFilter, which includes theUpdatedAfterparameter. These changes are consistent with the new filtering mechanism and ensure that the list with custom orderBy uses the updated filtering logic.Also applies to: 158-158
Line range hint
1-220: Overall assessment: Well-implemented changesThe changes in this file consistently implement the new filtering mechanism using
UpdatedAfterinstead ofCreatedAfter. This aligns well with the PR objectives and should help prevent conflicts with other dialogs. The implementation is consistent throughout the file, and code readability has been improved.Key points:
- Introduction of
updatedAfteranddefaultFiltervariables.- Consistent use of
defaultFilterin all API calls.- Improved code readability with additional empty lines.
No major issues were found, and the changes appear to be well-tested and implemented.
tests/k6/tests/enduser/dialogSearch.js (2)
48-49: Approved: Important change to useUpdatedAfterinstead ofCreatedAfterThis change aligns with the PR objective to use
UpdatedAfterinstead ofChangedAfterin the default filter. By focusing on the update time rather than the creation time, this modification helps prevent conflicts with other dialogs, addressing the intermittent issues in e2e tests.As
defaultFilteris used throughout the file, this change will affect all test cases, ensuring consistent behavior across the test suite.
Line range hint
50-291: Verify test cases with the updated filterWhile the change from
CreatedAftertoUpdatedAfterin thedefaultFiltershould be a drop-in replacement, it's crucial to ensure that all test cases still function as expected. ThedefaultFilteris used throughout the file, so this change affects multiple test scenarios.To verify the impact of this change, we can run a script to check if all test cases are still passing:
This script will run the k6 test and check for any failures. If you'd like me to create a more detailed verification process or if you need assistance in interpreting the results, please let me know.
tests/k6/tests/serviceowner/testdata/01-create-dialog.js (2)
3-3: LGTM: Import statement added correctly.The new import for
sentinelValuefrom the config file is properly placed and will be used in the test data. This addition aligns with the PR objective of implementing a sentinel mechanism for detecting unpurged dialogs.
Line range hint
1-278: LGTM: Changes enhance test data without disruption.The modifications to this file are minimal yet impactful. The addition of the
sentinelValueto thesearchTagsarray enhances the test data's capability to detect unpurged dialogs after a test run, aligning perfectly with the PR objectives. These changes have been implemented cleanly, without disrupting the existing structure or purpose of the test data.The overall impact is positive, improving the robustness of the testing framework while maintaining backwards compatibility with existing tests.
elsand
changed the title
|
Description
This hopefully fixes an intermittent issue in e2e tests where leftovers from previously running tests sometimes were not cleaned up properly, and interfered with subsequent tests.
Verification
Summary by CodeRabbit
Release Notes
Documentation
New Features
sentinelValuefor tracking unpurged dialogs.Improvements
Bug Fixes