Skip to content

Conversation

@kpowderly
Copy link
Contributor

@kpowderly kpowderly commented Oct 31, 2025

Description

Please see: https://github.com/SpecterOps/bloodhound-enterprise/pull/699 for more insight.

Since we're adding slow-go-tests back to Pull Requests, I had to fix the failing tests first.

I noticed our Slow Integration Tests are failing. These tests run against real Postgres instances (via pgtestdb) and each test opens a connection pool. When tests run in parallel, the pools add up quickly and can exceed Postgres’s max_connections limit, causing flaky “too many connections” errors. For now, we should probably try to avoid using t.Parallel() in any slow-integration tests.

Motivation and Context

Resolves <TICKET_OR_ISSUE_NUMBER>

Why is this change required? What problem does it solve?

How Has This Been Tested?

Passing pipeline

Screenshots (optional):

Types of changes

  • Chore (a change that does not modify the application functionality)

Checklist:

Summary by CodeRabbit

  • Tests
    • Updated test execution configuration to improve test reliability and consistency.

@kpowderly kpowderly self-assigned this Oct 31, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

The PR removes t.Parallel() calls from integration tests in three files, converting these tests from parallel execution to sequential execution. All test logic, assertions, and setup remain unchanged.

Changes

Cohort / File(s) Summary
Graphify service integration tests
cmd/api/src/services/graphify/ingestnodes_integration_test.go, cmd/api/src/services/graphify/tasks_integration_test.go
Removed t.Parallel() calls from TestIngestNode and multiple integration test functions, changing them to run sequentially
Datapipe daemon integration tests
cmd/api/src/daemons/datapipe/pipeline_integration_test.go
Removed t.Parallel() calls from TestDeleteData_Sourceless, TestDeleteData_SourceKinds, and TestDeleteData_All

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • All changes are homogeneous—removing identical t.Parallel() directives across multiple test files
  • No logic modifications, assertions, or control flow changes
  • Simple, repetitive edits with minimal complexity

Possibly related PRs

Suggested reviewers

  • superlinkx
  • cweidenkeller
  • mvlipka

Poem

🐰 Tests once danced in parallel grace,
Now they queue in ordered place,
Sequential steps, one by one,
Harmony's where races were once run!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title references the Jira ticket (BED-6503) but is vague and generic about the actual change; it doesn't clearly convey that the change involves removing t.Parallel() from integration tests to fix flaky test failures. Consider a more specific title like 'Remove t.Parallel() from slow integration tests to prevent connection exhaustion' to better describe the actual change and fix being implemented.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description provides context, motivation, and a clear explanation of the problem being solved. The template sections are mostly filled out, though the Jira ticket reference is incomplete in the 'Motivation and Context' section.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BED-6503-pipeline-controller

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ae696a1 and c00eb18.

📒 Files selected for processing (3)
  • cmd/api/src/daemons/datapipe/pipeline_integration_test.go (0 hunks)
  • cmd/api/src/services/graphify/ingestnodes_integration_test.go (0 hunks)
  • cmd/api/src/services/graphify/tasks_integration_test.go (0 hunks)
💤 Files with no reviewable changes (3)
  • cmd/api/src/services/graphify/tasks_integration_test.go
  • cmd/api/src/daemons/datapipe/pipeline_integration_test.go
  • cmd/api/src/services/graphify/ingestnodes_integration_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build BloodHound Container Image / Build and Package Container
  • GitHub Check: run-analysis
  • GitHub Check: run-tests
  • GitHub Check: build-ui

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kpowderly kpowderly force-pushed the BED-6503-pipeline-controller branch from 19e87d0 to 010aa52 Compare November 7, 2025 18:19
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3fcf28e and 8fd3dff.

📒 Files selected for processing (1)
  • .github/workflows/run-prepare-for-review.yml (3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.

Applied to files:

  • .github/workflows/run-prepare-for-review.yml
📚 Learning: 2025-07-17T15:10:25.757Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1652
File: .github/workflows/run-main-tests.yml:46-47
Timestamp: 2025-07-17T15:10:25.757Z
Learning: The `stbernard deps` command is not deprecated and remains a functional command in the BloodHound codebase. It's implemented in `packages/go/stbernard/command/deps/deps.go` and is used to ensure workspace dependencies are up to date by calling `yarn.InstallWorkspaceDeps`.

Applied to files:

  • .github/workflows/run-prepare-for-review.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build BloodHound Container Image / Build and Package Container
  • GitHub Check: run-prepare-for-review
  • GitHub Check: build-ui
  • GitHub Check: run-tests
🔇 Additional comments (3)
.github/workflows/run-prepare-for-review.yml (3)

17-17: Clear naming improvement for workflow purpose.

The renamed workflow and job better reflect the comprehensive code review preparation process (moving from "Static Code Analysis" to "Run Prepare For Code Review"). This aligns well with the expanded scope of the pipeline.

Also applies to: 29-29


35-36: Fetch-depth: 0 appropriate for code generation and synchronization steps.

Full repository history is necessary for the stbernard code generation and module synchronization operations that follow. This is a standard requirement for comprehensive code review preparation workflows.


54-54: Step naming improves clarity.

The renamed steps ("Install Dependencies" and "Run Static Analysis") provide clearer intent while maintaining the same functional behavior with stbernard commands.

Also applies to: 58-58

@kpowderly kpowderly force-pushed the BED-6503-pipeline-controller branch from ae696a1 to c00eb18 Compare November 13, 2025 22:23
@kpowderly kpowderly requested a review from computator November 14, 2025 19:21
@kpowderly kpowderly merged commit d5f3360 into main Nov 17, 2025
9 checks passed
@kpowderly kpowderly deleted the BED-6503-pipeline-controller branch November 17, 2025 18:02
@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants