mirror of
https://github.com/hanzoai/team.git
synced 2026-09-30 01:57:35 +00:00
master
19
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
987e51c176 |
ci: fix startup_failure in nightly race and weekly workflows (#36198)
* ci: fix startup_failure in nightly race and weekly workflows Add id-token: write permission to both server-ci-nightly-race.yml and server-ci-weekly.yml. The reusable server-test-template.yml declares id-token: write in its permissions block (needed for FIPS Docker Hub login via OIDC). GitHub requires that caller workflows grant at least the permissions declared by any reusable workflow they invoke — regardless of whether the steps using those permissions are skipped at runtime. Both new workflows only declared contents: read, causing immediate startup_failure with zero jobs created. Release Note ```release-note NONE ``` * ci: remove unused id-token: write from server-test-template The template declared id-token: write but nothing in the workflow uses OIDC token exchange — the FIPS Docker Hub login uses plain secrets (DOCKERHUB_USERNAME/DOCKERHUB_TOKEN), not an OIDC identity token. Removing it from the template means caller workflows (nightly race, weekly, and the main server-ci) no longer need to grant id-token: write either, following the principle of least privilege. This is the actual root cause fix: the previous commit added id-token to the callers as a workaround, but the real issue was the template requesting a permission it never uses. --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
c27e14a00b |
ci: add yamllint workflow to detect duplicate YAML keys (#36010)
* ci: add yamllint workflow to detect duplicate YAML keys Add a yamllint check for workflow files to catch duplicate keys that YAML parsers silently accept but GitHub Actions rejects on the default branch. Release Note NONE * ci: address review feedback on yamllint workflow - Add permissions: contents: read (least privilege) - Bump checkout to v6.0.2 with persist-credentials: false - Remove pip install step (yamllint is pre-installed on ubuntu-22.04) --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
74c67d3c22 |
fix(tests): re-enable 10 flaky tests across 12 JIRA tickets (#36159)
* fix(tests): widen ChannelMemberHistory time windows (MM-67041, MM-67037)
The 100ms query window (GetMillis()-100 to GetMillis()+100) is too
tight under CI load — DB writes can lag >100ms, causing the history
record to fall outside the window. Widen to ±10s and capture start
time before the operation.
* fix(tests): use require.Eventually for DND status restore (MM-63533)
Replace time.Sleep(3s) + instant assert with require.Eventually polling
(15s timeout, 500ms interval). The recurring task runs every 1s but can
lag under CI load, making the fixed sleep unreliable.
* fix(tests): fix PostReminder timing and increase WS timeout (MM-60329)
Set target time 1s in the future instead of now, giving the reminder
processor a clear tick to pick it up. Increase WebSocket wait from 5s
to 15s for CI load tolerance.
* fix(tests): add error checks and poll for file infos (MM-46902)
File info association with posts can be async. Replace instant
assertion with require.Eventually polling. Also add proper error
checking on UploadFile and CreatePost calls.
* fix(tests): fix thread membership race and WS timeout (MM-41285)
Replace instant GetUserThread assertion with require.Eventually polling
(10s timeout) since mention counts may not propagate immediately after
AddChannelMemberWithRootId. Increase WebSocket timeout from 2s to 15s.
* fix(tests): use far-future timestamps in GetUptoNSizeFileTime (MM-53905)
PermanentDeleteBatch only cleans up to current time, and parallel tests
can create file infos that interfere with the cumulative size calc.
Extend the delete window and use timestamps 1 hour in the future to
ensure these test files are always 'most recent'.
* fix(tests): use far-future timestamps in GetNthRecentPostTime (MM-64438)
GetNthRecentPostTime queries by global position across all posts.
Parallel tests creating posts shift the Nth position. Using timestamps
1 hour in the future ensures these test posts are always most recent.
* fix(tests): space reply timestamps for unread count test (MM-41797)
makeSomePosts creates all posts without explicit CreateAt, so they can
share the same millisecond. MarkAsRead(rootPost.CreateAt) then marks
replies as read too, returning 0 instead of 2. Fix by overwriting
reply timestamps to be 1s after the root post.
* test: re-enable MM-62895 and MM-61041 to probe for failures
Remove t.Skip for two long-dormant flaky tests to collect actual
failure data over 1-2 weeks of rebasing. Based on fullyparallel
branch so these run with every CI pass.
- MM-62895: TestUpdateOAuthApp (14mo, empty Jira, unknown root cause)
- MM-61041: TestAutocompleteUsersInChannel (17mo, fragile integration test)
* test: add MM-64687 shared channel tests to probe
Remove t.Skip("MM-64687") from 5 shared channel sync tests to collect
failure data. The underlying sync mechanism was completely rewritten in
#35619 (ChannelMemberHistory cursor-based sync), so the original race
condition may no longer exist. Probing for 1-2 weeks before deciding
whether to permanently re-enable in #35762.
Tests:
- api4/shared_channel_metadata_test.go (4 subtests)
- app/shared_channel_membership_sync_self_referential_test.go (1 subtest)
* test: re-enable TestResetPassword (stale skip from old build server)
The skip comment says 'should be investigated' — the test uses the same
inbucket mail pattern as other working tests (e.g. TestInviteUsersToTeam).
The 'old build server changes' it references are long resolved.
* fix(tests): clean up far-future posts in GetNthRecentPostTime
Per review feedback from mgdelacroix: defer PermanentDelete for all
far-future posts so they don't leak into other tests that query
GetNthRecentPostTime. The file_info_store test (GetUptoNSizeFileTime)
already had cleanup via defer PermanentDelete.
* ci: TEMPORARY enable race detector for PR testing (revert after)
* fix(tests): add mutex to fix data race in shared channel sync test
The -race detector caught a data race in Test_4 (Sync failure and
recovery): the OnBatchSync/OnIndividualSync callbacks write to
successfulSyncs from the HTTP handler goroutine while assert.Eventually
reads it from the test goroutine. Add sync.Mutex to protect all
accesses to the shared slice.
* fix(test): add mutex to TestSharedChannelPostMetadataSync for race safety
The syncedPosts/syncedPostsServerA/syncedPostsServerB slices are
written by OnPostSync callbacks running on HTTP server goroutines
and read by require.Eventually + assertions on the test goroutine.
Without synchronization, the race detector flags concurrent access.
Add sync.Mutex per subtest to protect all slice reads and writes,
matching the pattern used in TestSharedChannelMembershipSyncSelfReferential.
* Revert "ci: TEMPORARY enable race detector for PR testing (revert after)"
This reverts commit b9af0e9003de924e8c731b43fef59549ba61c0e7.
* fix: address review feedback from wiggin77
- Remove eager-evaluated len(infos) from require.Eventually message
(was always 0 since infos is nil at call time)
- Log errors from PermanentDelete in test cleanup
* fix: rename shadowed err variable in test cleanup
Use delErr to avoid shadowing the outer err variable, which
triggers govet's shadow checker in CI.
---------
Co-authored-by: Hanzo Dev <[email protected]>
|
||
|
|
449f70c7c6 |
ci: move FIPS and binary params tests to weekly schedule (#36036)
* ci: move FIPS and binary params tests to weekly schedule Move low-regression-risk test suites out of the per-push/PR Server CI workflow into a new weekly scheduled workflow (Monday 1am EST / 5am UTC): - Postgres with binary parameters (1x 8-core runner) - Postgres FIPS sharded tests (4x 8-core runners + merge job) - mmctl FIPS tests (1x 8-core runner) This reduces the per-push 8-core runner demand from 14 concurrent jobs to 5 (4 Postgres shards + 1 ES), which should significantly reduce queue times that currently reach 90+ minutes during peak hours. The weekly workflow also supports workflow_dispatch for manual triggering when urgent FIPS or binary parameter verification is needed. #### Release Note ```release-note NONE ``` * ci: move coverage shards to 2-core runners Add a 'runner' input to server-test-template.yml (defaults to ubuntu-latest-8-cores for backward compatibility) and set coverage shards to ubuntu-22.04 (2-core). Coverage is non-blocking (allow-failure: true) so longer runtime doesn't impact PR feedback. Estimated ~20-30 min per shard on 2-core vs ~7-9 min on 8-core, but frees 4 more 8-core slots per push. Combined with the FIPS/binary-params weekly move, per-push 8-core demand drops from 14 → 4 (just the Postgres test shards + ES v8). * ci: decouple race detector from binary params, add nightly race job The race detector was accidentally bundled with binary params via the fullyparallel=false → RACE_MODE coupling in the test template. These test different things: - Binary params: Postgres driver binary encoding mode - Race detector: Go data race detection Changes: - Add explicit 'race-enabled' input to server-test-template.yml - Remove implicit fullyparallel→race coupling from template - Binary params now runs with fullyparallel: true (default) - New server-ci-nightly-race.yml runs -race nightly at 2am EST on ubuntu-22.04 (2-core) to avoid 8-core contention * ci: add push trigger for release-* branches to weekly workflow FIPS and binary params validation must run automatically on release branch pushes, not just on the weekly schedule. Without this trigger, release branches would lose FIPS/binary coverage entirely. * ci: use ET instead of EST in schedule comments Cron runs at fixed UTC times regardless of DST. Use ~ET to avoid implying exact EST/EDT correspondence. * ci: restore conditional FIPS on per-push, unshard weekly FIPS Per review feedback from @lieut-data: 1. Restore FIPS jobs in server-ci.yml with conditional execution: run on all pushes (master/release) and on PRs when go.mod changed or branch name contains 'fips'. This ensures Go upgrades and explicit FIPS work get immediate feedback. 2. Remove sharding from weekly FIPS — no speed pressure on a weekly schedule, so a single unsharded job is simpler (eliminates the 4-shard matrix + merge job). 3. Restore gomod-changed detection step in the go job. Both per-push (conditional, unsharded) and weekly (unconditional, unsharded) FIPS runs use single jobs now, reducing complexity. * ci: restore FIPS sharding for PR runs, remove from push events FIPS tests in server-ci.yml now only trigger on PRs where the branch name contains 'fips' or go.mod changed. Sharding (4 shards + merge) restored for fast iteration on FIPS-related PRs. Regular FIPS coverage provided by the weekly workflow (unsharded). This addresses lieut-data's review feedback to restore sharding where it matters most: during active PR iteration. * ci: add explicit permissions to weekly and nightly workflows Set minimum required permissions (contents: read) on both new workflow files per review feedback. Reusable workflows called via 'uses' inherit the caller's permissions. * ci: keep coverage shards on 8-core runners Comment out the 2-core runner override for coverage shards per Eva's feedback. Coverage stays on the default 8-core runners. --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
26f5fdc448 |
ci: pin enterprise repo to explicit commit hash (#35957)
* ci: pin enterprise repo to explicit commit hash Introduces an enterprise.pin file that explicitly pins the enterprise commit the server is built and tested against. This replaces the implicit HEAD checkout, eliminating the CI race condition for all cross-repo changes. Changes: - enterprise.pin: pinned enterprise commit SHA - server/Makefile: new bump-enterprise target to update the pin * ci: fix bump-enterprise error handling and path resolution - Add set -e so recipe fails fast if git rev-parse fails - Use $(ROOT)/../enterprise.pin instead of ../enterprise.pin to resolve path relative to Makefile location, not CWD * fix: add missing 'all' to .PHONY declaration in Makefile Pre-existing issue flagged by CodeRabbit — the 'all' target (line 199) was never declared phony. Out of scope for the enterprise.pin change but trivial to fix. * ci: trigger Enterprise CI re-run --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
960b86df94 |
fix: add explicit permission grant in team members test (#36007)
* fix: add explicit permission grant in team members test TestGetTeamMembersForUserRoleDataSanitization was relying on a permission side-effect leaked from concurrent tests. Under fullyparallel, another test temporarily adds PermissionReadOtherUsersTeams to system_user role, which the team admin subtest accidentally benefits from. Under sequential execution (binary parameters mode), no concurrent test leaks this permission, so the team admin correctly gets 403. Fix by explicitly granting ReadOtherUsersTeams in the subtest setup, matching the pattern used in adjacent subtests. Release Note NONE * fix: remove explanatory comment per review feedback --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
8650f0ab58 |
fix: remove duplicate allow-failure input in server test template (#36004)
The allow-failure input was defined twice in the workflow_call inputs, causing GitHub Actions to reject the workflow with 0 jobs on master push. Duplicate was introduced in #35743 merge. Release Note NONE Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
2bdfd46340 |
ci: re-enable server test coverage with 4-shard parallelism (#35743)
* ci: re-enable server test coverage with 4-shard parallelism The test-coverage job was disabled due to OOM failures when running all tests with coverage instrumentation in a single process. Re-enable it by distributing the workload across 4 parallel runners using the shard infrastructure from the sharding PRs. Changes: - Replace disabled single-runner test-coverage with 4-shard matrix - Add merge-coverage job to combine per-shard cover.out files - Upload merged coverage to Codecov with server flag - Skip per-shard Codecov upload when sharding is active - Add coverage profile merging to run-shard-tests.sh for multi-run shards - Restore original condition: skip coverage on release branch PRs - Keep fullyparallel=true (fast within each shard) - Keep continue-on-error=true (coverage never blocks PRs) * fix: disable fullyparallel for coverage shards t.Parallel() + t.Setenv() panics kill entire test binaries under fullyparallel mode. With 4-shard splitting, serial execution within each shard should still be fast enough (~15 min). We can re-enable fullyparallel once the incompatible tests are fixed. * fix: add checkout to coverage merge job for Codecov file mapping Codecov needs the source tree to map coverage data to files. Without checkout, the upload succeeds but reports 0% coverage because it can't associate cover.out lines with source files. * ci: add codecov.yml and retain merged coverage artifact Add codecov.yml with: - Project coverage: track against parent commit, 1% threshold, advisory - Patch coverage: 50% target for new code, advisory (warns, doesn't block) - Ignore generated code (retrylayer, timerlayer, serial_gen, mocks, storetest, plugintest, searchtest) — these inflate the denominator from 146K to 100K statements, rebasing coverage from 36% to 53% - PR comments on coverage changes with condensed layout Save merged cover.out as artifact with 30-day retention (~3.5MB/run). 90-day retention was considered (~6.3GB total vs ~2.1GB at 30 days) but deferred to keep storage costs low. #### Release Note ```release-note NONE ``` * ci: add codecov.yml to exclude generated code and enable PR comments (#35748) * ci: add codecov.yml to exclude generated code and enable PR comments Add Codecov configuration to improve coverage signal quality: - Exclude generated code from coverage denominator: - store/retrylayer (~10k stmts, auto-generated retry wrappers) - store/timerlayer (~14k lines, auto-generated timing wrappers) - *_serial_gen.go (serialization codegen) - **/mocks (mockery-generated mocks) - Exclude test infrastructure: - store/storetest (~63k lines, test helpers not production code) - plugin/plugintest (plugin test helpers) - Exclude thin wrappers: - model/client4.go (~4k stmts, HTTP client methods tested via integration) - Enable PR comments with condensed layout - Set project threshold at 0.5% drop tolerance - Set patch target at 60% for new/changed lines This rebases the effective coverage metric from ~33.8% to ~43% by removing ~50k non-production statements from the denominator, giving a more accurate picture of actual test coverage. * Update codecov.yml --------- Co-authored-by: Jesse Hallam <[email protected]> * fix: bump upload-artifact to v7 and add client4.go to codecov ignore - Align upload-artifact pin with the rest of the workflow (v4 → v7) - Add model/client4.go to codecov.yml ignore list as documented in PR description * fix(ci): address Jesse review feedback on coverage sharding - Remove client4.go from codecov ignore list (coverage is meaningful) - Remove historical comment block above test-coverage job - Set fullyparallel back to true (safe per-shard since each runs different packages; parallel test fixes tracked in #35751) - Replace merge-coverage job with per-shard Codecov uploads using flags parameter; configure after_n_builds: 4 so Codecov waits for all shards before reporting status - Add clarifying comment in run-shard-tests.sh explaining intra-shard coverage merge (multiple gotestsum runs) vs cross-shard merge (handled natively by Codecov) - Simplify codecov.yml: remove verbose comments, use informational status checks, streamlined ignore list * fix(ci): set fullyparallel back to false for coverage shards Coverage shards 1-3 failed with hundreds of test failures because fullyparallel: true causes panics and races in tests that use t.Setenv, os.Setenv, and os.Chdir without parallel-safe alternatives. The parallel-safety fixes are tracked in a separate PR chain: - #35746: t.Setenv → test hooks - #35749: os.Setenv → parallel-safe alternatives - #35750: os.Chdir → t.Chdir - #35751: flip fullyparallel: true (final step) Once that chain merges, fullyparallel can be enabled for coverage too. * fix(ci): split fullyparallel and allow-failure into separate inputs Previously fullyparallel controlled both parallel test execution AND continue-on-error, meaning disabling parallelism also made coverage failures blocking. Split into two independent inputs: - fullyparallel: controls ENABLE_FULLY_PARALLEL_TESTS (test execution) - allow-failure: controls continue-on-error (advisory vs blocking) Coverage shards now run with fullyparallel: true (Claudio's original approach) and allow-failure: true (failures don't block PRs until parallel-safety fixes land in #35746 → #35751). * ci: use per-flag after_n_builds for server and webapp coverage Replace the global after_n_builds: 2 with per-flag values: - server: after_n_builds: 4 (one per shard) - webapp: after_n_builds: 1 (single merged upload) Tag the webapp Codecov upload with flags: webapp so each flag independently waits for its expected upload count. This prevents Codecov from firing notifications with incomplete data when the webapp upload arrives before all server shards complete. Addresses review feedback from @esarafianou. * fix: consolidate codecov config into .github/codecov.yml Move all codecov configuration into the existing .github/codecov.yml instead of introducing a duplicate file at the repo root. Merges improvements from the root file (broader ignore list, informational statuses, require_ci_to_pass: false) while preserving the webapp flag from the original config. Updates after_n_builds to 5 (4 server + 1 webapp). --------- Co-authored-by: Jesse Hallam <[email protected]> Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
a850c66f9c |
ci: disable fullyparallel for binary parameters job (#35995)
Binary parameters tests run unsharded on a single runner. With fullyparallel enabled, all ~755 api4 tests run concurrently, causing resource exhaustion (too many server instances, WebSocket hubs, and DB connections). The test binary gets killed after 11 minutes with no individual test failures — just overwhelmed resources. Disabling fullyparallel for this specific job lets binary parameters tests pass while we evaluate moving them to a nightly/weekly schedule. Co-authored-by: Hanzo Dev <[email protected]> |
||
|
Pavel ZemanandGitHub
|
c66f4ec153 |
Revert "Strip remote_id field from user patch API requests (#35910)" (#35996)
This reverts commit
|
||
|
|
e03d609871 |
ci: enable fullyparallel mode for server tests (#35816)
* ci: enable fullyparallel mode for server tests Replace os.Setenv, os.Chdir, and global state mutations with parallel-safe alternatives (t.Setenv, t.Chdir, test hooks) across 37 files. Refactor GetLogRootPath and MM_INSTALL_TYPE to use package-level test hooks instead of environment variables. This enables gotestsum --fullparallel, allowing all test packages to run with maximum parallelism within each shard. * ci: split fullyparallel from continue-on-error in workflow template - Add new boolean input 'allow-failure' separate from 'fullyparallel' - Change continue-on-error to use allow-failure instead of fullyparallel - Update server-ci.yml to pass allow-failure: true for test coverage job - Allows independent control of parallel execution and failure tolerance * fix: protect TestOverrideLogRootPath with sync.Mutex for parallel tests - Replace global var TestOverrideLogRootPath with mutex-protected functions - Add SetTestOverrideLogRootPath() and getTestOverrideLogRootPath() functions - Update GetLogRootPath() to use thread-safe getter - Update all test files to use SetTestOverrideLogRootPath() with t.Cleanup() - Fixes race condition when running tests with t.Parallel() * fix: configure audit settings before server setup in tests - Move ExperimentalAuditSettings from UpdateConfig() to config defaults - Pass audit config via app.Config() option in SetupWithServerOptions() - Fixes audit test setup ordering to configure BEFORE server initialization - Resolves CodeRabbit's audit config timing issue in api4 tests * fix: implement SetTestOverrideLogRootPath mutex in logger.go The previous commit updated test callers to use SetTestOverrideLogRootPath() but didn't actually create the function in config/logger.go, causing build failures across all CI shards. This commit: - Replaces the exported var TestOverrideLogRootPath with mutex-protected unexported state (testOverrideLogRootPath + testOverrideLogRootMu) - Adds exported SetTestOverrideLogRootPath() setter - Adds unexported getTestOverrideLogRootPath() getter - Updates GetLogRootPath() to use the thread-safe getter - Fixes log_test.go callers that were missed in the previous commit * fix(test): use SetupConfig for access_control feature flag registration InitAccessControlPolicy() checks FeatureFlags.AttributeBasedAccessControl at route registration time during server startup. Setting the flag via UpdateConfig after Setup() is too late — routes are never registered and API calls return 404. Use SetupConfig() to pass the feature flag in the initial config before server startup, ensuring routes are properly registered. * fix(test): restore BurnOnRead flag state in TestRevealPost subtest The 'feature not enabled' subtest disables BurnOnRead without restoring it via t.Cleanup. Subsequent subtests inherit the disabled state, which can cause 501 errors when they expect the feature to be available. Add t.Cleanup to restore FeatureFlags.BurnOnRead = true after the subtest completes. * fix(test): restore EnableSharedChannelsMemberSync flag via t.Cleanup The test disables EnableSharedChannelsMemberSync without restoring it. If the subtest exits early (e.g., require failure), later sibling subtests inherit a disabled flag and become flaky. Add t.Cleanup to restore the flag after the subtest completes. * Fix test parallelism: use instance-scoped overrides and init-time audit config Replace package-level test globals (TestOverrideInstallType, SetTestOverrideLogRootPath) with fields on PlatformService so each test gets its own instance without process-wide mutation. Fix three audit tests (TestUserLoginAudit, TestLogoutAuditAuthStatus, TestUpdatePasswordAudit) that configured the audit logger after server init — the audit logger only reads config at startup, so pass audit settings via app.Config() at init time instead. Also revert the Go 1.24.13 downgrade and bump mattermost-govet to v2.0.2 for Go 1.25.8 compatibility. * Fix audit unit tests * Fix MMCLOUDURL unit tests * Fixed unit tests using MM_NOTIFY_ADMIN_COOL_OFF_DAYS * Make app migrations idempotent for parallel test safety Change System().Save() to System().SaveOrUpdate() in all migration completion markers. When two parallel tests share a database pool entry, both may race through the check-then-insert migration pattern. Save() causes a duplicate key fatal crash; SaveOrUpdate() makes the second write a harmless no-op. * test: address review feedback on fullyparallel PR - Use SetLogRootPathOverride() setter instead of direct field access in platform/support_packet_test.go and platform/log_test.go (pvev) - Restore TestGetLogRootPath in config/logger_test.go to keep MM_LOG_PATH env var coverage; test uses t.Setenv so it runs serially which is fine (pvev) - Fix misleading comment in config_test.go: code uses t.Setenv, not os.Setenv (jgheithcock) * fix: add missing os import in post_test.go The os import was dropped during a merge conflict resolution while burn-on-read shared channel tests from master still use os.Setenv. --------- Co-authored-by: wiggin77 <[email protected]> Co-authored-by: Mattermost Build <[email protected]> Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
e756da5b86 |
fix: prevent sql.DB connectionCleaner race and harden flaky tests (#35891)
* fix: prevent sql.DB connectionCleaner race with testify mock diffing The previous fix (#35881) moved store.Close()/th.Shutdown() before AssertExpectations, but the race actually occurs earlier — during mock.Called() when RegisterDBCollector fires inside initConnection. Root cause: testify's Arguments.Diff() unconditionally calls fmt.Sprintf("%v", *sql.DB) on every recorded argument (mock.go:976), which uses reflect to read sql.DB internal fields. The connectionCleaner goroutine concurrently writes to those same fields, triggering a DATA RACE under Go 1.25's stricter race detector. Fix: Set ConnMaxLifetimeMilliseconds=0 and ConnMaxIdleTimeMilliseconds=0 in test SqlSettings before initConnection. This prevents the connectionCleaner goroutine from starting at all — no concurrent writer means no race. Connection pool cleanup serves no purpose in tests. For store_test.go: settings are set directly before SqlStore creation. For platform tests: settings are set in setupTestHelper's config before sqlstore.New() is called via platform.New(). * fix: harden flaky TestScheduleOnceSequential and TestGroupStore tests TestScheduleOnceSequential: Replace fixed 300ms sleep with require.Eventually polling (5s timeout, 50ms interval). Under the race detector, execution is significantly slower and 25+ scheduled jobs may not complete within a fixed 300ms window. TestGroupStore/GetGroups: Use unique uid-prefixed display names for test groups to avoid collisions with groups created by other parallel subtests sharing the same database. Search queries and result checks updated to use the uid prefix. * fix: address CodeRabbit review feedback - Restore mixed-case search coverage in TestGroupStore by using mixed-case query strings (uid + "-GrOuP-3") to verify case-insensitive matching behavior. - Strengthen exactly-once validation in TestScheduleOnceSequential by checking callback count == 1 instead of > 0, ensuring duplicate executions are caught. --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
c3041bc444 |
fix: resolve DATA RACE in TestReplicaLagQuery, TestInvalidReplicaLagDataSource, and TestMetrics (#35881)
These three tests have been failing deterministically on every master build since Go 1.25.8 was merged (PR #35817, March 27). The failures only manifest on master because PR CI does not run with the -race flag. ## Root Cause The race is between Go's database/sql connection pool and testify's mock assertion logic: 1. When a *sql.DB connection pool is opened, Go spawns a background connectionCleaner goroutine that periodically runs connectionCleanerRunLocked(), which acquires the DB's internal mutex via atomic.CompareAndSwapInt32. 2. The tests pass a *sql.DB argument to mock expectations via RegisterDBCollector(). When testify's AssertExpectations() runs, it uses reflect to diff the recorded call arguments, which reads the internal fields of the *sql.DB struct (including the mutex state and connection pool fields) via reflect.Value.Int() and reflect.typedmemmove(). 3. Go 1.25 ships a stricter race detector that catches this concurrent read (reflect in the test goroutine) vs write (atomic CAS in the connectionCleaner goroutine) on the same memory. Go 1.24's race detector did not flag this pattern. The race existed latently in the code for a long time. Go 1.25 simply made it detectable. ## Fix Close/shut down the store (and all its background goroutines, including the sql.DB connection cleaner) BEFORE calling AssertExpectations. This ensures there is no concurrent writer when testify uses reflect to inspect the *sql.DB arguments. - TestReplicaLagQuery: move store.Close() before AssertExpectations (was previously deferred, running after AssertExpectations) - TestInvalidReplicaLagDataSource: replace defer store.Close() with explicit store.Close() at end of test - TestMetrics/ensure_advanced_metrics_have_database_metrics: call th.Shutdown(t) before AssertExpectations (Shutdown was previously registered via t.Cleanup, running after the test function returns) Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
00296f9b49 |
ci: shard server Postgres tests into 4 parallel runners (#35739)
* ci: add test sharding plumbing to server CI Add infrastructure for upcoming test sharding without changing behavior: - Add shard-index and shard-total inputs to server-test-template.yml (defaults preserve existing single-runner behavior) - Add timing cache restore step (activates only when shard-total > 1) - Add merge-postgres-test-results job to server-ci.yml that: - Merges JUnit XML reports from shard artifacts - Saves timing data cache for future shard balancing - Handles both single-artifact and multi-shard scenarios - Add .gitignore entries for timing cache and shard work files * ci: shard server Postgres tests into 4 parallel runners Extract sharding logic into standalone, tested scripts and enable 4-shard parallel test execution for server Postgres CI: Scripts: - server/scripts/shard-split.js: Node.js bin-packing solver that assigns test packages to shards using timing data from previous runs. Two-tier strategy: light packages (<2min) whole, heavy packages (api4, app) split at individual test level. - server/scripts/run-shard-tests.sh: Multi-run wrapper that calls gotestsum directly for each package group with -run regex filters. - server/scripts/shard-split.test.js: 8 test cases covering round-robin fallback, timing-based balancing, heavy package splitting, JUnit XML fallback, and enterprise package separation. Workflow changes: - server-test-template.yml: Add shard splitting step that discovers test packages and runs the solver. Modified Run Tests step to use wrapper script when sharding is active. - server-ci.yml: Add 4-shard matrix to test-postgres-normal. Update merge job artifact patterns for shard-specific names. Performance: 7.2 min with timing cache vs 62.5 min baseline = 88% wall-time improvement. First run without cache uses JUnit XML fallback or round-robin, then populates the cache for subsequent runs. * fix: raise heavy package threshold to 5 min to preserve test isolation sqlstore integrity tests scan the entire database and fail when other packages' test data is present. At 182s, sqlstore was just over the 120s threshold and getting split at test level. Raising to 300s keeps only api4 (~38 min) and app (~15 min) as heavy — where the real sharding gains are — while sqlstore, elasticsearch, etc. stay whole and maintain their test isolation guarantees. * ci: only save test timing cache on default branch PR branches always restore from master's timing cache via restore-keys prefix matching. Timing data is stable day-to-day so this eliminates cache misses on first PR runs and reduces cache storage. * ci: skip FIPS tests on PRs (enterprise CI handles compile check) Per review feedback: the enterprise CI already runs a FIPS compile check on every PR. Running the full FIPS test suite on PRs is redundant since it uses the identical test suite as non-FIPS — the only FIPS-specific failure mode is a build failure from non-approved crypto imports, which the enterprise compile check catches. Full FIPS tests continue to run on every push to master. * fix: address review feedback on run-shard-tests.sh - Remove set -e so all test runs execute even if earlier ones fail; track failures and exit with error at the end (wiggin77) - Remove unused top-level COVERAGE_FLAG variable (wiggin77) - Fix RUN_IDX increment position so report, json, and coverage files share the same index (wiggin77) - Update workflow comment: heavy threshold is 5 min, not 2 min (wiggin77) * style: use node: prefix for built-in fs module in shard-split.js * fix: avoid interpolating file paths into generated shell script Read shard package lists from files at runtime instead of interpolating them into the generated script via printf. This prevents theoretical shell metacharacter injection from directory names, as flagged by DryRun Security. * fix(ci): rename merged artifact to match server-ci-report glob The merged artifact was named postgres-server-test-logs-merged which does not match the *-test-logs pattern in server-ci-report.yml, causing Postgres test results to be missing from PR/commit reports. Also pins junit-report-merger to exact version 7.0.0 for supply chain safety. * fix(ci): pass RACE_MODE env into Docker container RACE_MODE was set on the host runner but never included in the docker run --env list. The light-package path worked because the heredoc expanded on the host, but run-shard-tests.sh reads RACE_MODE at runtime inside the container where it was unset. This caused heavy packages (api4, app) to silently lose -race detection. * fix(ci): discover new tests in heavy packages not in timing cache Tests not present in the timing cache (newly added or renamed) would not appear in any shard -run regex, causing them to silently skip. After building items from the cache, run go test -list to discover current test names and assign any cache-missing tests to shards via the normal bin-packing algorithm with a small default duration. * fix(ci): add missing line continuation backslash in docker run The previous --env FIPS_ENABLED line was missing a trailing backslash after adding --env RACE_MODE, causing docker run to see a truncated command and fail with "requires at least 1 argument". * fix(ci): add setup-go step for shard test discovery go test -list in shard-split.js runs on the host runner via execSync, but Go is only available inside the Docker container. Without this step, every invocation fails silently and new-test discovery is a no-op. Adding actions/setup-go before the shard split step ensures the Go toolchain is available on the host. --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
7ac96df7b4 |
Fix nil pointer dereference in UpdateUser (MATTERMOST-SERVER-VF) (#35717)
* Fix nil pointer dereference in UpdateUser after store update Add nil check on userUpdate result from userService.UpdateUser to prevent panic when the store returns nil unexpectedly. This fixes a nil pointer dereference that occurs when accessing userUpdate.New after the store update call. Sentry: MATTERMOST-SERVER-VF (14 events) * Add unit test for nil userUpdate guard in UpdateUser Test verifies that when the store returns (nil, nil) from Update, the app layer returns an appropriate error instead of panicking with a nil pointer dereference. * fix: gofmt user_test.go * fix: split nil checks per review feedback, add parallel test execution Separate userUpdate==nil from userUpdate.New==nil with distinct error detail strings for easier debugging. Add mainHelper.Parallel(t) to test for consistency with other mock-based tests. Addresses review feedback from @JulienTant and @coderabbitai. --------- Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
8733a3e5ce |
ci: cache prepackaged plugins in mmctl tests (#35720)
* ci: cache prepackaged plugins in mmctl tests Cache the prepackaged_plugins directory between mmctl test runs using actions/cache. The cache key is derived from a SHA-256 hash of the PLUGIN_PACKAGES lines in server/Makefile, so the cache automatically invalidates whenever any plugin version is bumped — no manual key updates needed. Uses actions/cache output (cache-hit) to skip the download step entirely on cache hits. Saves ~1-2 minutes per mmctl test run by avoiding repeated plugin downloads. * fix: align plugin cache key with actual downloaded packages Address CodeRabbit review: cache key was derived from all 15 plugins in the Makefile, but the download only fetched jira plugin. Now both the hash and the download use the same MMCTL_PLUGIN_PACKAGES value, defined once, preventing stale cache or unnecessary invalidation. * Revert "fix: align plugin cache key with actual downloaded packages" This reverts commit 73303864bf1c066a49f649bcdab5c2201b38dd1f. * Revert "ci: cache prepackaged plugins in mmctl tests" This reverts commit acd1ec757afda3e6f5e4907c3dc92aa7765563f1. * prepackage the existing test plugin to speed up tests * fix: use public key for plugin signature verification in test --------- Co-authored-by: Jesse Hallam <[email protected]> Co-authored-by: Hanzo Dev <[email protected]> |
||
|
|
8add33a051 |
ci: skip server CI for docs-only changes (#35719)
Add path exclusions for markdown files, NOTICE.txt, and CHANGELOG.md to the pull_request trigger. This prevents the full 60-minute CI suite from running when a PR only changes documentation. Exclusions added: - server/**/*.md - server/NOTICE.txt - server/CHANGELOG.md Co-authored-by: Hanzo Dev <[email protected]> |
||
|
Pavel ZemanandGitHub
|
93776d7b51 |
Update grafana dashboards in local dev docker environment (#34266)
* Enabling Prometheus, Grafana, Loki and Promtail running as containers by default in local dev environments * Updating the Grafana dashboards available in the local dev environment * Revert "Enabling Prometheus, Grafana, Loki and Promtail running as containers by default in local dev environments" This reverts commit 3a252a8383a965d1e32f921d5b997e4add53bc47. * this config change is not needed anymore |
||
|
Pavel ZemanandGitHub
|
68860f707a | Enabling Prometheus, Grafana, Loki and Promtail running as containers by default in local dev environments (#34263) |