Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration test : Sync mode + db reset option + cleaning #8376

Merged
merged 9 commits into from
Nov 7, 2024

Conversation

guillim
Copy link
Contributor

@guillim guillim commented Nov 6, 2024

Run the CI integrationin sync mode
and add the option to run it without db reset
cleaning all the useless integration test

@guillim guillim added scope: backend Issues that are affecting the backend side only -PR: awaiting review labels Nov 6, 2024
@guillim guillim self-assigned this Nov 6, 2024
@guillim guillim removed the request for review from magrinj November 6, 2024 16:59
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR significantly modifies the integration testing setup and removes numerous test files to streamline the testing process.

  • Added maxWorkers: 1 in jest-integration.config.ts to run tests sequentially, preventing race conditions
  • Modified CI workflow in .github/workflows/ci-server.yaml to use new test:integration:reset task and upload reset logs
  • Split integration test commands in project.json into two variants: with and without DB reset
  • Removed 25+ integration test files covering various resolvers (API keys, attachments, audit logs, etc.) as part of test cleanup
  • Enabled previously skipped JWT auth strategy tests by changing xdescribe to describe

30 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile

packages/twenty-server/project.json Outdated Show resolved Hide resolved
packages/twenty-server/project.json Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing all attachment resolver tests leaves critical CRUD operations untested. Consider keeping essential test cases or replacing with more focused tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing all audit log integration tests without replacement could make it harder to catch bugs in audit log functionality. Consider keeping core audit log tests or documenting where this test coverage has moved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing all noteTarget resolver tests leaves critical CRUD operations untested. Consider keeping core test cases or replacing with equivalent coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing this entire test suite leaves timeline activities untested. Consider keeping core functionality tests or replacing with more focused test cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing all view field integration tests creates a significant test coverage gap. Need confirmation that these scenarios are covered elsewhere or rationale for removing this coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing this entire test suite eliminates coverage for critical view sort operations. Consider keeping core functionality tests even during cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing this entire test suite leaves critical view functionality untested. These tests covered CRUD operations, soft deletion, and data integrity checks. Consider keeping a simplified version if cleaning up, or ensure equivalent coverage exists elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing all webhook integration tests without replacement or relocation could leave critical webhook functionality untested. Ensure equivalent test coverage exists elsewhere.

@charlesBochet charlesBochet merged commit d9c0530 into main Nov 7, 2024
18 of 19 checks passed
@charlesBochet charlesBochet deleted the ci-sync branch November 7, 2024 16:22
Copy link

github-actions bot commented Nov 7, 2024

Thanks @guillim for your contribution!
This marks your 4th PR on the repo. You're top 8% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-PR: awaiting review scope: backend Issues that are affecting the backend side only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants