Skip to content

improve e2e emulated tests readability - #600

Closed
lionelvillard wants to merge 1 commit into
llm-d:mainfrom
lionelvillard:break-the-monolith
Closed

improve e2e emulated tests readability#600
lionelvillard wants to merge 1 commit into
llm-d:mainfrom
lionelvillard:break-the-monolith

Conversation

@lionelvillard

Copy link
Copy Markdown
Collaborator

see below

Signed-off-by: Lionel Villard <villard@us.ibm.com>
Copilot AI review requested due to automatic review settings January 16, 2026 22:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors the e2e test structure by renaming "saturation-based" to "emulated" throughout the codebase to improve test readability and better reflect that these tests run in an emulated environment rather than being specific to saturation-based scaling mode.

Changes:

  • Renamed package from e2esaturation to e2eemulated and updated directory references
  • Refactored test infrastructure setup into separate, well-documented functions
  • Created a new test suite file (e2e_emulated_suite_test.go) separating test execution from infrastructure setup
  • Added comprehensive README documentation for the emulated e2e tests

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/utils/e2eutils.go Updated regex pattern to match new e2e-emulated directory name
test/e2e-emulated/infra_setup.go Refactored infrastructure setup into modular functions, moved shared constants and clients from test file, changed package name to e2eemulated
test/e2e-emulated/e2e_saturation_test.go Updated package name, removed duplicate constants and client initialization code (moved to infra_setup.go), added empty HPA Mode test suite placeholder
test/e2e-emulated/e2e_emulated_suite_test.go New file containing test suite runner and setup/teardown hooks
test/e2e-emulated/README.md Added comprehensive documentation covering test architecture, configuration, usage, and troubleshooting
Makefile Updated comment and test path to reference e2e-emulated instead of e2e-saturation-based
Comments suppressed due to low confidence (3)

test/e2e-emulated/infra_setup.go:46

  • The variable name 'WVAConfigMapName' uses inconsistent casing. All other constants in this section use camelCase (e.g., 'saturationConfigMapName', 'controllerNamespace'), but this one uses PascalCase. For consistency and to follow Go conventions for private constants, it should be 'wvaConfigMapName'.
    test/e2e-emulated/e2e_saturation_test.go:56
  • This comment is misleading. The function doesn't just validate reactive saturation-based scaling behavior; based on the empty test suite above it (line 53-54), it appears this is part of a broader test suite that includes HPA Mode tests as well. The comment should be more general or clarify that it applies specifically to the "Saturation Mode" test suite.
    test/e2e-emulated/e2e_saturation_test.go:54
  • This empty test suite for HPA Mode lacks implementation. Having a test suite without any test cases can be confusing and may cause issues with test execution. Either implement the test cases or remove this empty suite declaration, or add a comment explaining why it's empty (e.g., placeholder for future tests).

@asm582

asm582 commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator

Hmm, the resolve conflict button is greyed out for me. I don't know how Co-Pilot will resolve conflicts.

@lionelvillard

Copy link
Copy Markdown
Collaborator Author

actually this is my PR (with Claude's help), not copilot's :-) . I will rebase.

@asm582

asm582 commented Feb 2, 2026

Copy link
Copy Markdown
Collaborator

actually this is my PR (with Claude's help), not copilot's :-) . I will rebase.

Thanks, once it is rebased, we can merge.

@mamy-CS

mamy-CS commented Feb 26, 2026

Copy link
Copy Markdown
Collaborator

Closing because this PR was aiming to improve a deprecated test suite; the future is test/e2e, which already has the right structure.

@mamy-CS mamy-CS closed this Feb 26, 2026
@lionelvillard
lionelvillard deleted the break-the-monolith branch June 18, 2026 13:16
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.

4 participants