fix: Increase log line count for tests#2445
Conversation
Increased the maximum number of log lines to check in various test cases. This change was made to accommodate potentially longer log outputs during test execution, preventing false negatives due to insufficient log history being examined. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Introduced a new script, `hack/check-e2e-test-naming.sh`, to enforce a naming convention for end-to-end tests. This script validates that test function names start with specific prefixes (e.g., `TestGithub*`, `TestGitlab*`) or include "Concurrency" to ensure they can be correctly categorized and executed by CI jobs. The Makefile was updated to include this new linting step, and the GitHub Actions workflow configuration (`.github/workflows/e2e.yaml`) was adjusted to correctly pass necessary secrets and variables for testing against Bitbucket Cloud and GitHub. A minor adjustment was also made to the `github_second_controller` test selection in `hack/gh-workflow-ci.sh` to include "Others" tests. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
🔍 PR Lint Feedback
|
Summary of ChangesHello @chmouel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and maintainability of the project's end-to-end testing suite. It improves the diagnostic capabilities of tests by expanding the log data available for analysis and introduces a structured approach to E2E test organization through a new naming convention and automated validation. These changes streamline the testing process and facilitate easier identification and resolution of issues. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses test flakiness by increasing the number of log lines scanned in several E2E tests. It also introduces a new linting script to enforce a naming convention for E2E tests and refactors existing test names to comply. This is a great step towards better test organization.
My review includes a suggestion to improve the new bash script for better performance and readability. I've also pointed out an opportunity to replace the hardcoded log line count with a shared constant to improve maintainability across the test suite, aligning with the guideline to define magic numbers as constants. The PR title could also be updated to reflect the addition of the test naming linter, as it's a significant part of the changes.
| orphaned_tests=() | ||
| while IFS= read -r test; do | ||
| [[ -z "${test}" ]] && continue | ||
| if ! echo "${test}" | grep -qE "${valid_pattern}"; then | ||
| orphaned_tests+=("${test}") | ||
| fi | ||
| done <<< "${all_tests}" |
There was a problem hiding this comment.
The while loop to find orphaned tests can be made more efficient and concise. Instead of iterating through each test and running grep in a loop, you can use grep -v to filter out all valid tests at once and populate the orphaned_tests array directly using mapfile.
| orphaned_tests=() | |
| while IFS= read -r test; do | |
| [[ -z "${test}" ]] && continue | |
| if ! echo "${test}" | grep -qE "${valid_pattern}"; then | |
| orphaned_tests+=("${test}") | |
| fi | |
| done <<< "${all_tests}" | |
| mapfile -t orphaned_tests < <(echo "${all_tests}" | grep -vE -- "${valid_pattern}" | grep -v '^$') |
| defer f() | ||
| reg := regexp.MustCompile(".*successfully fetched git-clone task from default configured catalog Hub") | ||
| maxLines := int64(100) | ||
| maxLines := int64(1000) |
There was a problem hiding this comment.
The magic number 1000 for the maximum log lines is now used in many test files. To improve maintainability and ensure consistency, consider defining a shared constant for this value (e.g., in the test/pkg/wait package). This would also be a good opportunity to standardize the variable name, which currently varies (e.g., maxLines, numLines, logLinesToCheck) across different tests.
References
- Magic numbers should be defined as constants for better readability and maintainability.
There was a problem hiding this comment.
Pull request overview
This PR increases the log line count parameter in E2E tests from various values (20, 50, 100) to a standardized 1000 lines, and introduces a naming convention enforcement mechanism for test functions to ensure proper CI job partitioning.
Changes:
- Standardized log line counts to 1000 across multiple E2E tests to prevent test failures due to insufficient log inspection
- Renamed five test functions to use the "TestOthers" prefix for proper CI job routing
- Added a linting script to enforce E2E test naming conventions
- Updated CI configuration to route "Others"-prefixed tests appropriately
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/gitlab_delete_tag_event_test.go | Increased log line count from 100 to 1000 |
| test/github_skip_ci_test.go | Increased log line count from 100 to 1000 in two locations |
| test/github_push_retest_test.go | Increased log line count from 20 to 1000 |
| test/github_pullrequest_test.go | Increased log line count from 100 to 1000 in two locations |
| test/github_pullrequest_privaterepository_test.go | Increased log line count from 50 to 1000 |
| test/gitea_test.go | Increased log line count to 1000 in four locations (from 20 and 100) |
| test/repository_webhook_test.go | Renamed test function to TestOthersRepositoryCreation |
| test/repo_validation_test.go | Renamed test function to TestOthersRepoValidation |
| test/invalid_event_test.go | Renamed three test functions with TestOthers prefix |
| hack/check-e2e-test-naming.sh | Added new linting script to validate E2E test naming conventions |
| hack/gh-workflow-ci.sh | Updated to route "Others"-prefixed tests to github_second_controller job |
| Makefile | Integrated e2e naming lint check into the main lint target |
| .github/workflows/e2e.yaml | Alphabetically reordered environment variables for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request introduces improvements to E2E test naming enforcement, updates CI workflow configuration, and increases log line checks in tests for better diagnostics. The changes ensure that E2E tests follow a strict naming convention for proper CI partitioning, update how secrets and variables are set in the CI workflow, and improve test reliability by allowing more log lines to be checked for expected output.
E2E Test Naming Enforcement:
hack/check-e2e-test-naming.shand integrated it into thelinttarget in theMakefileto enforce that all E2E test functions follow specific naming conventions. This helps ensure tests are correctly partitioned into CI jobs. [1] [2]test/invalid_event_test.go,test/repo_validation_test.go, andtest/repository_webhook_test.goto use theTestOthers*prefix for compliance with the new naming convention. [1] [2] [3] [4] [5]hack/gh-workflow-ci.shscript to includeOthers-prefixed tests in thegithub_second_controllerjob.CI Workflow Configuration Updates:
.github/workflows/e2e.yamlto ensure secrets and variables are set correctly for Bitbucket and GitHub E2E tests. [1] [2]👨🏻 Linked Jira
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.