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

chore(ci): refactor e2e package and ci tests to speed up ci #404

Merged
merged 13 commits into from
May 31, 2024

Conversation

MSevey
Copy link
Member

@MSevey MSevey commented May 29, 2024

Overview

Closes #403

This PR does the following:

  • Splits the e2e tests into separate packages so that they can be easily run individually
  • Updates the ci to run 4 groups of tests, which allows us to:
    • Parallelize the CI more to run faster
    • See more easily which tests are failing
    • See more easily which tests are taking the longest

We could also now enable the tests that are passing as required, which I highly recommend. So that we have better protection on future changes.

Summary by CodeRabbit

  • New Features

    • Enhanced testing workflow with dynamic test packages and timeouts.
  • Bug Fixes

    • Corrected file paths in end-to-end tests to ensure proper resource loading.
  • Refactor

    • Renamed packages from basic to system across multiple test files for better organization.
    • Updated function calls to use standardized assertion methods from the e2e package.
  • Chores

    • Introduced a timeout variable in the Makefile for dynamic test timeout configuration.
    • Adjusted package declarations and function names to align with the new package structure.

Copy link
Contributor

coderabbitai bot commented May 29, 2024

Walkthrough

This update introduces several improvements and modifications across the codebase. Key changes include updates to the GitHub Actions workflow for testing, enhancements to the Makefile for dynamic timeout settings, and various package renamings in the end-to-end tests. Additionally, functions have been refactored to use shared assertion utilities, and some test paths and environment variable handling have been adjusted.

Changes

File(s) Summary
.github/workflows/knuu_testing.yml Added merge_group trigger, introduced a strategy block with a matrix for test packages and timeouts, and modified make test to use dynamic values.
Makefile Added a timeout variable set to 120m for dynamic timeout in go test.
e2e/basic/probe_test.go Updated file path in TestProbe function.
e2e/bittwister/bittwister_test.go, e2e/bittwister/main_test.go Changed package declaration to bittwister and added test initialization.
e2e/system/... Renamed package from basic to system in multiple test files; updated function calls to use e2e package utilities.
pkg/builder/kaniko/utils_test.go Replaced testFilePerms with os.ModePerm for file permissions in TestCreateTarGz.

Poem

In the code, a rabbit hops with glee,
Through workflows and tests, changes we see.
Packages renamed, functions refined,
Dynamic timeouts, all well-aligned.
With each commit, the system grows,
A garden of code, where innovation flows. 🌸🐇


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@MSevey MSevey mentioned this pull request May 29, 2024
@MSevey MSevey marked this pull request as ready for review May 30, 2024 00:49
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

Commits Files that changed from the base of the PR and between 74e6413 and e638a7d.
Files selected for processing (16)
  • .github/workflows/knuu_testing.yml (2 hunks)
  • Makefile (1 hunks)
  • e2e/assert_cleanups.go (3 hunks)
  • e2e/assert_create.go (3 hunks)
  • e2e/basic/probe_test.go (1 hunks)
  • e2e/bittwister/bittwister_test.go (1 hunks)
  • e2e/bittwister/main_test.go (1 hunks)
  • e2e/system/build_from_git_test.go (2 hunks)
  • e2e/system/env_to_json_test.go (4 hunks)
  • e2e/system/external_file_test.go (1 hunks)
  • e2e/system/file_test.go (1 hunks)
  • e2e/system/file_test_image_cached_test.go (4 hunks)
  • e2e/system/files_to_volumes_cm_test.go (10 hunks)
  • e2e/system/folder_test.go (2 hunks)
  • e2e/system/folder_test_image_cached_test.go (3 hunks)
  • e2e/system/main_test.go (1 hunks)
Files skipped from review due to trivial changes (7)
  • Makefile
  • e2e/assert_cleanups.go
  • e2e/bittwister/bittwister_test.go
  • e2e/system/build_from_git_test.go
  • e2e/system/external_file_test.go
  • e2e/system/file_test.go
  • e2e/system/main_test.go
Additional comments not posted (11)
e2e/bittwister/main_test.go (1)

1-19: The TestMain function setup in the bittwister package is well-implemented. It ensures that the knuu package is initialized before any tests are run and properly handles any initialization errors. The use of logrus for logging the scope and fatal errors is appropriate. Exiting with the test run's exit value is also correctly implemented.

.github/workflows/knuu_testing.yml (2)

5-23: The modifications to the workflow file introduce a matrix strategy for running tests, which is a great way to parallelize and manage different test configurations. The inclusion of specific packages with designated timeouts is a thoughtful approach to managing test execution time. This setup should effectively help in speeding up the CI process as intended.


43-46: The dynamic configuration of the make test command using matrix variables for package paths and timeouts is a robust way to handle test executions across different configurations. This change should contribute significantly to the flexibility and efficiency of the CI process.

e2e/system/folder_test.go (1)

22-31: The refactoring of the test to use AssertCreateInstanceNginxWithVolumeOwnerWithoutCommit for creating the instance without committing it immediately allows for more granular control over the test flow, especially with subsequent operations like AddFolder and Commit. This change enhances the test's clarity and control over the instance lifecycle.

e2e/system/folder_test_image_cached_test.go (2)

29-42: The test TestFolderCached effectively demonstrates the use of concurrency with sync.WaitGroup and goroutines to handle multiple instances. The update to use AssertCreateInstanceNginxWithVolumeOwner from the e2e package ensures consistency and reuse of test setup logic across different tests. This is a good practice in maintaining clean and maintainable test code.


48-48: Proper cleanup using AssertCleanupInstances is crucial in tests, especially when dealing with multiple instances like in this test. This ensures that resources are properly released after tests, preventing potential resource leaks or conflicts in subsequent tests.

e2e/system/file_test_image_cached_test.go (2)

27-27: The use of AssertCreateInstanceNginxWithVolumeOwner for instance creation in the cached file test is consistent with other tests, promoting code reuse and maintainability. This standardization is beneficial for maintaining a uniform approach across different tests.


47-47: Ensuring proper cleanup in tests is essential, and the use of AssertCleanupInstances here follows best practices in test hygiene. This is particularly important in a test environment where multiple instances are created and manipulated.

e2e/assert_create.go (1)

Line range hint 22-45: Splitting the instance creation into two separate functions (AssertCreateInstanceNginxWithVolumeOwnerWithoutCommit and AssertCreateInstanceNginxWithVolumeOwner) is a good practice. It allows for more flexible test design, where tests can opt to manipulate the instance state further before committing. This change enhances both the flexibility and clarity of the test code.

e2e/system/env_to_json_test.go (1)

Line range hint 19-136: The enhancements to environment variable handling in the test improve its functionality and clarity.

e2e/system/files_to_volumes_cm_test.go (1)

48-48: The updates to use the e2e package for function calls and cleanup enhance the test's modularity and maintainability.

Also applies to: 114-114, 160-160, 191-191, 243-243, 272-272, 324-324, 356-356

e2e/basic/probe_test.go Show resolved Hide resolved
mojtaba-esk
mojtaba-esk previously approved these changes May 30, 2024
Copy link
Contributor

@mojtaba-esk mojtaba-esk left a comment

Choose a reason for hiding this comment

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

lgtm!
left a question

tty47
tty47 previously approved these changes May 30, 2024
@MSevey MSevey dismissed stale reviews from tty47 and mojtaba-esk via 4299aaf May 30, 2024 15:27
@MSevey MSevey requested review from tty47 and mojtaba-esk May 30, 2024 15:27
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: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e638a7d and 4299aaf.

Files selected for processing (1)
  • e2e/system/build_from_git_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • e2e/system/build_from_git_test.go

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: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4299aaf and adab8b2.

Files selected for processing (2)
  • e2e/bittwister/bittwister_test.go (1 hunks)
  • e2e/system/folder_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • e2e/bittwister/bittwister_test.go
  • e2e/system/folder_test.go

@MSevey MSevey enabled auto-merge May 30, 2024 16:10
mojtaba-esk
mojtaba-esk previously approved these changes May 30, 2024
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: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between adab8b2 and 5649887.

Files selected for processing (8)
  • Makefile (2 hunks)
  • e2e/basic/probe_test.go (1 hunks)
  • e2e/bittwister/main_test.go (1 hunks)
  • e2e/system/file_test_image_cached_test.go (4 hunks)
  • e2e/system/files_to_volumes_cm_test.go (10 hunks)
  • e2e/system/folder_test.go (2 hunks)
  • e2e/system/main_test.go (1 hunks)
  • pkg/builder/kaniko/utils_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • pkg/builder/kaniko/utils_test.go
Files skipped from review as they are similar to previous changes (7)
  • Makefile
  • e2e/basic/probe_test.go
  • e2e/bittwister/main_test.go
  • e2e/system/file_test_image_cached_test.go
  • e2e/system/files_to_volumes_cm_test.go
  • e2e/system/folder_test.go
  • e2e/system/main_test.go

Copy link
Contributor

@mojtaba-esk mojtaba-esk left a comment

Choose a reason for hiding this comment

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

LGTM!

@MSevey MSevey added this pull request to the merge queue May 31, 2024
Merged via the queue into main with commit 20f3ce9 May 31, 2024
9 of 11 checks passed
@MSevey MSevey deleted the sevey/ci-test-plit branch May 31, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split out tests, CI takes way too long
3 participants