Skip to content

chore: Don't truncate database tables between tests #2572

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JanCizmar
Copy link
Contributor

No description provided.

@JanCizmar JanCizmar requested a review from Anty0 October 11, 2024 07:50
@JanCizmar JanCizmar mentioned this pull request Oct 11, 2024
3 tasks
Copy link
Contributor

This PR is stale because it has been open for 30 days with no activity.

Copy link

coderabbitai bot commented May 19, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get basic information about the PR
gh pr view 2572 --json title,body,state,additions,deletions,changedFiles,baseRefName,headRefName,mergeable

Length of output: 344


🏁 Script executed:

#!/bin/bash
# Get a list of files with merge conflicts
git checkout jancizmar/dont-truncate-in-tests || git fetch origin jancizmar/dont-truncate-in-tests && git checkout jancizmar/dont-truncate-in-tests
git merge main || true
git diff --name-only --diff-filter=U

Length of output: 550


🏁 Script executed:

#!/bin/bash
# Get the list of files changed in the PR
gh pr view 2572 --json files --jq '.files[].path'

Length of output: 9857


🏁 Script executed:

#!/bin/bash
# Fetch the entire PR diff to understand the changes
gh pr view 2572 --json files --jq '.files[].patch' | head -n 300

Length of output: 166


🏁 Script executed:

#!/bin/bash
# Get more detailed PR information
gh pr view 2572 

# Now look at a specific test file to understand the nature of changes
# The PR title mentions "Don't truncate database tables between tests"
gh pr view 2572 --json files --jq '.files[].path' | grep -i testbase

# Let's examine a core testing utility file
cat backend/app/src/test/kotlin/io/tolgee/AbstractSpringTest.kt || echo "File not found"

Length of output: 584


🏁 Script executed:

#!/bin/bash
# Find test base files or classes
find backend -type f -name "*.kt" | grep -i test | grep -i base || echo "No base test files found"

# Also search for the term "truncate" to find the relevant database operations
find backend -type f -name "*.kt" | xargs grep -l "truncate" 2>/dev/null || echo "No files with truncate found"

# Look at a specific test file to understand the structure
find backend -type f -name "*.kt" | grep -i test | head -n 1 | xargs cat || echo "No test files found"

Length of output: 1259


🏁 Script executed:

#!/bin/bash
# Try to find the abstract controller test class that many tests extend
find backend -type f -name "*.kt" | xargs grep -l "AbstractControllerTest" | head -n 1 | xargs cat

# Look for files related to test database setup
find backend -type f -name "*.kt" | xargs grep -l "truncateTable\|cleanDb\|clearDb" 2>/dev/null || echo "No files with db cleanup found"

# Get a sample of changes from one of the test files to understand the nature of modifications
gh pr view 2572 --json commits --jq '.commits[0].oid' | xargs gh api /repos/tolgee/tolgee-platform/commits/ | jq -r '.files[0].patch' | head -n 50

Length of output: 839


🏁 Script executed:

#!/bin/bash
# Look for the AbstractControllerTest class in the testing module
find backend -name "AbstractControllerTest.kt" -type f | xargs cat || echo "File not found"

# Get first few commit messages to understand the nature of changes
gh pr view 2572 --json commits --jq '.commits[].messageHeadline' | head -n 10

# Let's check if we can find any database setup/cleanup code in the test files
find backend -type f -name "*.kt" | xargs grep -l "setUp\|tearDown\|database\|cleanDatabase" 2>/dev/null || echo "No relevant files found"

# Look for test utility classes related to database operations
find backend -type f -path "*/test/*" -name "*.kt" | xargs grep -l "TestDbConfig\|TestDbSetup" 2>/dev/null || echo "No test db config files found"

Length of output: 6713


@laz-001

I've analyzed PR #2572 "Don't truncate database tables between tests" to assess its feasibility. Here's what I found:

Merge Conflicts

The PR currently has conflicts with the main branch that need resolution. According to GitHub, the PR is in a CONFLICTING state with 113 changed files.

Scope & Complexity

  • The PR makes extensive changes: 588 additions, 131 deletions across 113 test files
  • It fundamentally changes how the test database is managed between tests
  • I found database cleanup code in files like CleanDbBeforeClass.kt, CleanDbBeforeMethod.kt, and CleanDbTestListener.kt

Potential Challenges

  1. Test Interdependency: Removing table truncation between tests could create dependencies between tests, making them less isolated and harder to debug
  2. Maintenance Complexity: Tests would need to be carefully designed to account for existing data
  3. Unpredictable Behavior: Tests might become flaky as their execution order could affect results

Alternative Approaches

Your suggested alternatives seem more targeted and potentially less disruptive:

  1. Opt-in/opt-out CleanDbBefore tags: Provides flexibility while maintaining test isolation where needed
  2. More transactional tests: Could improve performance without sacrificing isolation
  3. Uniqueness generators: Would help prevent data conflicts

Conclusion

Given the scope of changes, conflicts, and risk of test interdependency, I believe the PR in its current form would be challenging to complete and maintain. Your alternative approaches, especially the opt-in/opt-out tags, offer a more balanced solution that could achieve performance improvements with less risk.

@JanCizmar
Copy link
Contributor Author

Hey! I can rebase this, but there still will be lot of work to do, since the tests don't pass. However, truncating the tables has huge impact on the total test time. Truncate takes c.a. 500ms on my machine, which for 800 test is 400 seconds ~5min. 🤷

@JanCizmar
Copy link
Contributor Author

Yes, we must not use @Transactional in tests, beauase it behaves different and can lead to actual issues. @transactional on test actually creates a transaction level above the controller method. Then if you for example forget to create transaction on service layer, it can paas in the tests, but fail in the production.

That's why I consider @Transactional in tests anti-pattern.

To clarify: you have applied a working pattern you introduced to SOME classes (which pass now), but many classes need still to be updated with the new pattern, thus they pass.

So, essentially, it's grinding work, possibly with some adjustments here and there?

Exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant