chore(tests): using test suites and better test names where possible#6564
chore(tests): using test suites and better test names where possible#6564CommanderStorm merged 18 commits intolouislam:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates backend tests from flat test structures to organized test suites using describe() blocks, improving test output readability and organization. The changes consolidate duplicate tests and apply consistent naming conventions across all test files.
Key Changes:
- Introduced
describe()blocks to group related tests into logical suites - Improved test names following the pattern
function() [behavior] [condition] - Consolidated SQL_DATETIME_FORMAT tests from test-maintenance.js into test-util.js
- Reorganized file structure by moving mock-webhook.js to notification-providers subdirectory
- Updated import paths in monitor tests to reflect proper directory structure
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/backend-test/test-util.js | Added describe block "Server Utilities", improved test names, consolidated SQL_DATETIME_FORMAT tests from test-maintenance.js |
| test/backend-test/test-uptime-calculator.js | Wrapped all tests in "Uptime Calculator" describe block with nested "Worst case scenario" suite, improved test names |
| test/backend-test/test-migration.js | Simplified test names to be more descriptive without redundant "Database Migration" prefix |
| test/backend-test/test-maintenance.js | File deleted - tests consolidated into test-util.js |
| test/backend-test/test-domain.js | Added "Domain Expiry" describe block, updated mock-webhook import path |
| test/backend-test/test-cert-hostname-match.js | Added "Certificate Hostname Validation" describe block, improved test names |
| test/backend-test/notification-providers/mock-webhook.js | File moved from test/mock-webhook.js to better organize test utilities |
| test/backend-test/monitors/test-websocket.js | Fixed import paths (../../ to ../../../), improved test names for clarity |
| test/backend-test/monitors/test-tcp.js | Fixed import paths, improved test names, removed redundant JSDoc comments |
| test/backend-test/monitors/test-rabbitmq.js | Fixed import paths, improved test names |
| test/backend-test/monitors/test-postgres.js | Fixed import paths, improved test names |
| test/backend-test/monitors/test-mssql.js | Fixed import paths, improved test names for all query-related tests |
| test/backend-test/monitors/test-mqtt.js | Fixed import paths, improved test names with better descriptions of test scenarios |
| test/backend-test/monitors/test-grpc.js | Fixed import paths, improved test names for gRPC keyword matching tests |
| test/backend-test/monitor-conditions/test-operators.js | Added "Expression Operators" describe block, improved test names |
| test/backend-test/monitor-conditions/test-evaluator.js | Added "Expression Evaluator" describe block, improved test names |
| test/backend-test/README.md | Updated template and added guidance on writing tests with describe/test structure |
| server/model/domain_expiry.js | Added async keyword to getExpiryDate() method signature and corrected JSDoc return type |
| test("getExpiryDate() returns correct expiry date for .wiki domain with no A record", async () => { | ||
| await testDb.create(); | ||
| Notification.init(); |
There was a problem hiding this comment.
The testDb.create() and Notification.init() are called inside the first test, but subsequent tests may depend on this setup. If tests are run individually or in a different order, tests 2-4 will fail. Consider moving this setup to a beforeAll() hook at the describe block level to ensure proper test isolation and reliability.
| > [!TIP] | ||
| > Writing great tests is hard. | ||
| > | ||
| > You can make our live much simpler by following this guidance: |
There was a problem hiding this comment.
The word "live" should be "lives" (our lives).
| > You can make our live much simpler by following this guidance: | |
| > You can make our lives much simpler by following this guidance: |
|
|
||
| describe("Worst case scenario", () => { | ||
| test("handles year-long simulation with various statuses", { | ||
| skip: process.env.GITHUB_ACTIONS // Not stable on GitHub Actions" |
There was a problem hiding this comment.
The skip comment string is missing a closing comment delimiter. The string ends with a double quote but there's a comment starting with '//' that appears to be part of the string value. This should be either a complete string "Not stable on GitHub Actions" or if the comment marker is intentional, it needs proper handling.
| skip: process.env.GITHUB_ACTIONS // Not stable on GitHub Actions" | |
| skip: process.env.GITHUB_ACTIONS // Not stable on GitHub Actions |
| setTimeout(async () => { | ||
| Settings.stopCacheCleaner(); | ||
| await testDb.destroy(); | ||
| }, 200); |
There was a problem hiding this comment.
The test cleanup logic (Settings.stopCacheCleaner() and testDb.destroy()) has been moved from a suite-level .finally() block to inside the last test's setTimeout. This means if the last test doesn't run (e.g., due to filtering or early failure), the cleanup won't execute. Consider using afterAll() or a similar test suite lifecycle method for cleanup instead of setTimeout in the last test.
ℹ️ To keep reviews fast and effective, please make sure you’ve read our pull request guidelines
📝 Summary of changes done and why they are done
Currently, we don't use test suites, which leads to subpar readability of test output.
This PR migrates us to use them.