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: add E2E tests for sidecars #577

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

mojtaba-esk
Copy link
Member

@mojtaba-esk mojtaba-esk commented Oct 30, 2024

Closes #576

Summary by CodeRabbit

  • New Features

    • Introduced new test functions for validating command execution and file downloading within sidecar instances.
    • Added logging functionality tests for sidecar containers.
    • Established a new test suite for comprehensive end-to-end testing of the sidecars component.
  • Bug Fixes

    • Removed outdated sidecar logging tests to streamline the testing process.
  • Documentation

    • Enhanced error handling and validation across new test functions to improve reliability.

@mojtaba-esk mojtaba-esk requested a review from a team October 30, 2024 12:33
@mojtaba-esk mojtaba-esk self-assigned this Oct 30, 2024
Copy link
Contributor

coderabbitai bot commented Oct 30, 2024

Warning

Rate limit exceeded

@mojtaba-esk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 2 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between fcb8a19 and 8a30754.

Walkthrough

The changes in this pull request involve the removal of sidecar-related functionality from the end-to-end test suite, specifically eliminating the sidecarLogsTest struct and its methods from the e2e/basic/logs_test.go file. New tests have been introduced in the e2e/sidecars package, including functionalities to execute commands in a sidecar and download files from a running sidecar. Additionally, a new test suite has been created to facilitate testing for sidecars, ensuring comprehensive coverage of their functionalities.

Changes

File Change Summary
e2e/basic/logs_test.go Removed sidecarLogsTest struct and methods; removed TestLogsWithSidecar method.
e2e/sidecars/cmd_exec_test.go Added TestExecuteCommandInSidecar method to validate command execution in a sidecar container.
e2e/sidecars/file_test.go Added TestDownloadFileFromRunningSidecar method to validate file download from a sidecar.
e2e/sidecars/logs_test.go Added TestLogsWithSidecar method to validate logging functionality of a sidecar.
e2e/sidecars/sidecar_def_test.go Introduced testSidecar struct and its methods for managing sidecar instances.
e2e/sidecars/suite_setup_test.go Added a new test suite for sidecars, including setup and initialization methods.

Assessment against linked issues

Objective Addressed Explanation
Comprehensive e2e tests for sidecars (#576)
Execute a command in a sidecar (#576)
Read a file from a running sidecar (#576)

Possibly related PRs

Suggested reviewers

  • sysrex
  • aWN4Y25pa2EK
  • tty47
  • smuu

🐇 In the world of code, we hop and play,
Removing the old, making way for the new day.
With sidecars in tow, we test with delight,
Commands and files, all working just right!
So here's to the changes, let’s cheer and rejoice,
For every new feature, we’ll raise our voice! 🥕


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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration 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.

@celestia-bot celestia-bot requested review from MSevey and smuu October 30, 2024 12:33
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: 11

🧹 Outside diff range and nitpick comments (4)
e2e/sidecars/cmd_exec_test.go (1)

8-13: Add test documentation.

Consider adding a descriptive comment block above the test function to explain:

  • The purpose of the test
  • Expected behavior
  • Any prerequisites or assumptions

Add documentation like this:

+// TestExecuteCommandInSidecar validates that commands can be successfully
+// executed within a sidecar container. It verifies:
+// - Sidecar container creation and setup
+// - Command execution within the sidecar
+// - Output validation from the executed command
 func (s *Suite) TestExecuteCommandInSidecar() {
e2e/sidecars/logs_test.go (1)

10-10: Consider using a more distinctive test message.

The current message "Hello World" is generic. A more distinctive message would make it clearer that we're specifically testing sidecar logs.

-const expectedLogMsg = "Hello World"
+const expectedLogMsg = "Sidecar Test Log Message [timestamp: %d]"

This would also help verify log ordering if multiple messages are generated.

e2e/sidecars/file_test.go (1)

17-20: Consider using a timeout context and add documentation.

  1. Using context.Background() without a timeout could lead to hanging tests if the container operations fail to complete.
  2. The purpose of tail -f /dev/null should be documented for maintainability.

Consider these improvements:

-	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()

	s.Require().NoError(target.Build().SetImage(ctx, alpineImage))
-	s.Require().NoError(target.Build().SetArgs("tail", "-f", "/dev/null")) // Keep the container running
+	// Use tail -f to keep the container running indefinitely for testing
+	s.Require().NoError(target.Build().SetArgs("tail", "-f", "/dev/null"))
e2e/sidecars/sidecar_def_test.go (1)

18-39: Consider enhancing the instance name for better debugging.

While the initialization logic is solid, consider making the instance name more descriptive by including the test purpose or configuration in the name suffix.

-	s.instance, err = instance.New(namePrefix+"-sidecar-logs", sysDeps)
+	s.instance, err = instance.New(namePrefix+"-sidecar-"+s.getTestIdentifier(), sysDeps)

You'll need to add a method to generate a meaningful test identifier based on the sidecar's configuration or purpose.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f86e86c and 1ec8478.

📒 Files selected for processing (6)
  • e2e/basic/logs_test.go (0 hunks)
  • e2e/sidecars/cmd_exec_test.go (1 hunks)
  • e2e/sidecars/file_test.go (1 hunks)
  • e2e/sidecars/logs_test.go (1 hunks)
  • e2e/sidecars/sidecar_def_test.go (1 hunks)
  • e2e/sidecars/suite_setup_test.go (1 hunks)
💤 Files with no reviewable changes (1)
  • e2e/basic/logs_test.go
🔇 Additional comments (11)
e2e/sidecars/suite_setup_test.go (4)

1-13: LGTM! Well-organized imports.

The imports are logically grouped and all are necessary for the test suite implementation.


20-22: LGTM! Clean suite structure.

Good use of composition by embedding the base e2e.Suite.


24-26: LGTM! Standard suite runner setup.

Correctly implements the testify suite runner pattern.


28-45: 🛠️ Refactor suggestion

Consider using context with timeout and adding cleanup logic.

A few suggestions to improve the setup:

  1. Instead of using context.Background(), consider using a context with timeout to ensure proper test cleanup:
-	ctx = context.Background()
+	var cancel context.CancelFunc
+	ctx, cancel = context.WithTimeout(context.Background(), testTimeout)
+	defer cancel()
  1. Consider adding cleanup logic in a corresponding TearDownSuite method to ensure proper resource cleanup after tests.

Additionally, verify if the HandleStopSignal implementation properly cleans up resources:

e2e/sidecars/cmd_exec_test.go (1)

23-28: Verify sidecar implementation and improve container lifecycle.

A few concerns about the sidecar setup:

  1. The testSidecar type implementation needs verification
  2. Using sleep infinity isn't the best practice for keeping containers alive

Let's verify the sidecar implementation:

e2e/sidecars/logs_test.go (2)

1-8: LGTM: Package and imports are well-organized.

The package name appropriately reflects its purpose, and all imports are necessary and used within the code.


27-31: ⚠️ Potential issue

Verify the Alpine image constant and add error handling for sidecar operations.

The test uses an undefined alpineImage constant and could benefit from better error handling for sidecar operations.

Let's verify the Alpine image constant:

Add proper error handling and image constant:

+const alpineImage = "alpine:latest"
+
 func (s *Suite) TestLogsWithSidecar() {
     // ... existing code ...
+    defer func() {
+        if err := sidecar.Instance().Delete(ctx); err != nil {
+            s.T().Logf("Failed to delete sidecar instance: %v", err)
+        }
+    }()
     
     s.Require().NoError(target.Build().SetImage(ctx, alpineImage))
e2e/sidecars/file_test.go (1)

7-12: LGTM! Well-structured test setup.

The constants are well-named and the test method name clearly describes its purpose.

e2e/sidecars/sidecar_def_test.go (3)

1-9: LGTM!

The package name and imports are appropriate for the test file's purpose.


48-50: LGTM!

The Instance method is simple and serves its purpose well.


11-16: Verify the missing alpineImage constant.

The code references alpineImage in the Initialize method, but this constant is not defined in the current file.

Let's check if this constant is defined elsewhere:

e2e/sidecars/suite_setup_test.go Outdated Show resolved Hide resolved
e2e/sidecars/cmd_exec_test.go Show resolved Hide resolved
e2e/sidecars/cmd_exec_test.go Show resolved Hide resolved
e2e/sidecars/cmd_exec_test.go Outdated Show resolved Hide resolved
e2e/sidecars/logs_test.go Outdated Show resolved Hide resolved
e2e/sidecars/file_test.go Show resolved Hide resolved
e2e/sidecars/file_test.go Outdated Show resolved Hide resolved
e2e/sidecars/file_test.go Outdated Show resolved Hide resolved
e2e/sidecars/sidecar_def_test.go Outdated Show resolved Hide resolved
e2e/sidecars/sidecar_def_test.go Outdated Show resolved Hide resolved
Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

Looks good, I think we should refactor the common test code to clean things up and use the Retry helper instead of the sleep.

smuu
smuu previously approved these changes Nov 4, 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: 3

🧹 Outside diff range and nitpick comments (5)
e2e/sidecars/file_test.go (2)

7-12: Add test documentation.

Consider adding documentation to:

  • Describe the test's purpose and setup
  • Explain the significance of each constant
  • Document the expected behavior and test flow

Example:

+// TestDownloadFileFromRunningSidecar validates the ability to create and retrieve
+// files from a running sidecar instance. It sets up a target container with an
+// attached sidecar, creates a test file within the sidecar, and verifies the
+// file's content can be correctly retrieved.
 func (s *Suite) TestDownloadFileFromRunningSidecar() {
 	const (
+		// namePrefix uniquely identifies this test's resources
 		namePrefix  = "download-file-running-sidecar"
+		// fileContent is the test data to be written and verified
 		fileContent = "Hello World!"
+		// filePath is the location where the test file will be created
 		filePath    = "/hello.txt"
 	)

22-28: Verify sidecar readiness.

The sidecar setup should verify that the sidecar is fully operational before proceeding with tests.

 	s.Require().NoError(target.Sidecars().Add(ctx, sidecar))
 	s.Require().NoError(target.Execution().Start(ctx))
+
+	// Verify sidecar is ready
+	s.Require().Eventually(func() bool {
+		_, err := sidecar.Instance().Execution().ExecuteCommand(ctx, "ps", "aux")
+		return err == nil
+	}, 5*time.Second, 100*time.Millisecond, "sidecar failed to become ready")
e2e/sidecars/logs_test.go (3)

11-19: Add test documentation.

Consider adding a descriptive comment for the test function to explain its purpose and test scenario. This would help other developers understand the test's objectives and requirements.

+// TestLogsWithSidecar verifies that logs can be successfully retrieved from a running sidecar.
+// It creates a target instance with a sidecar that continuously outputs a message,
+// then validates that the expected log message can be read from the sidecar's log stream.
 func (s *Suite) TestLogsWithSidecar() {

24-29: Enhance sidecar command robustness.

The current command might flood logs without any rate limiting. Consider adding a timestamp and improving the command structure.

 sidecar := &testSidecar{
     StartCommand: []string{
         "sh", "-c",
-        fmt.Sprintf("while true; do echo '%s'; sleep 1; done", expectedLogMsg),
+        fmt.Sprintf("while true; do echo \"[$(date -Iseconds)] %s\"; sleep 1; done", expectedLogMsg),
     },
 }

38-51: Enhance log verification assertions.

While the basic log presence check is good, consider adding more comprehensive verifications:

  1. Verify multiple log lines are present
  2. Validate log format if timestamps are added
 s.Require().Eventually(func() bool {
     logStream, err := sidecar.Instance().Monitoring().Logs(ctx)
     if err != nil {
         return false
     }
     defer logStream.Close()

     logs, err := io.ReadAll(logStream)
     if err != nil {
         return false
     }

-    return strings.Contains(string(logs), expectedLogMsg)
+    logLines := strings.Split(strings.TrimSpace(string(logs)), "\n")
+    
+    // Ensure we have multiple log lines
+    if len(logLines) < 2 {
+        return false
+    }
+    
+    // Verify each line contains the expected message
+    for _, line := range logLines {
+        if !strings.Contains(line, expectedLogMsg) {
+            return false
+        }
+    }
+    
+    return true
 }, timeout, interval, "failed to get expected log message")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1ec8478 and fcb8a19.

📒 Files selected for processing (4)
  • e2e/sidecars/file_test.go (1 hunks)
  • e2e/sidecars/logs_test.go (1 hunks)
  • e2e/sidecars/sidecar_def_test.go (1 hunks)
  • e2e/sidecars/suite_setup_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • e2e/sidecars/suite_setup_test.go
🔇 Additional comments (7)
e2e/sidecars/file_test.go (2)

3-6: Add missing imports.

The import section is missing several required packages:

  • Testing framework imports (testing, require, assert)
  • Custom types and constants (Suite, testSidecar, alpineImage)

29-36: Implement accepted file handling improvements.

The previous review suggested several improvements that should be implemented:

  1. Use printf instead of echo for more reliable file creation
  2. Add cleanup of test files
  3. Improve error messages
e2e/sidecars/logs_test.go (1)

31-31: Verify alpineImage constant definition.

The alpineImage constant is used but not visible in this file. Ensure it's properly defined, either in this file or in a shared constants file.

✅ Verification successful

The alpineImage constant is properly defined and accessible

The constant is correctly defined in the same package (sidecars) within suite_setup_test.go as alpineImage = "alpine:3.20.3". Both files are in the same package, so the constant is accessible in logs_test.go.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for alpineImage constant definition
rg 'alpineImage\s*=' --type go

Length of output: 233


Script:

#!/bin/bash
# Check if the test file imports the correct package where alpineImage is defined
rg -B5 'alpineImage' e2e/sidecars/logs_test.go

# Get the package imports from the test file
rg '^import \(' -A 10 e2e/sidecars/logs_test.go

# Check the suite setup file content
cat e2e/sidecars/suite_setup_test.go

Length of output: 1188

e2e/sidecars/sidecar_def_test.go (4)

11-16: LGTM! Well-structured type definition with interface compliance check.

The struct is well-defined with necessary fields, and the interface compliance is properly verified.


41-49: LGTM! Basic validations are in place.

The PreStart method includes essential checks for instance initialization and start command configuration.


51-53: LGTM! Simple and correct getter implementation.


55-64: LGTM! Clone implementation is complete with proper StartCommand copying.

The method correctly clones both the instance and the StartCommand slice, addressing the previous review comment.

e2e/sidecars/file_test.go Outdated Show resolved Hide resolved
e2e/sidecars/sidecar_def_test.go Show resolved Hide resolved
e2e/sidecars/sidecar_def_test.go Show resolved Hide resolved
@mojtaba-esk mojtaba-esk requested a review from MSevey November 7, 2024 12:45
Copy link
Contributor

@tty47 tty47 left a comment

Choose a reason for hiding this comment

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

🔥

@mojtaba-esk mojtaba-esk merged commit 9bb917a into main Nov 8, 2024
13 checks passed
@mojtaba-esk mojtaba-esk deleted the mojtaba/576-E2E-tests-for-sidecars branch November 8, 2024 15:24
@coderabbitai coderabbitai bot mentioned this pull request Nov 8, 2024
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.

E2E tests for sidecars
4 participants