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

refactor: adds a E2ETestRunner for E2E tests #177

Merged
merged 5 commits into from
Mar 5, 2024
Merged

Conversation

jpower432
Copy link
Member

@jpower432 jpower432 commented Feb 27, 2024

Description

Update E2E test code to use a single class and fixture instead of a collection of functions.
Also removes negative test cases (which test specific failures and logging statements) that don't need to be tested in the container to the unit tests from the e2e tests.

Fixes #166

Type of change

  • Refactor

How has this been tested?

  • Updated unit tests run locally
  • E2E tests run locally

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

The negative test case exit codes and output can be easily verified
through unit test and should not change when not running in a container.
Saving E2E to verify functionality inside a container environment.

Signed-off-by: Jennifer Power <[email protected]>
For clarity, reference class variables by the class name

Signed-off-by: Jennifer Power <[email protected]>
@jpower432 jpower432 marked this pull request as ready for review February 29, 2024 00:33
Copy link
Contributor

@beatrizmcouto beatrizmcouto left a comment

Choose a reason for hiding this comment

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

LGTM. @jpower432 just wanted to understand the reasoning behind changing to a use a single class and fixture instead of a collection of functions. Both work but single class makes the code cleaner, is that is or another reasoning behind it? Thank you

@jpower432
Copy link
Member Author

jpower432 commented Mar 1, 2024

@beatrizmcouto Great question! A few reasons behind this change from my perspective:

  • It adds encapsulation for how the commands are built and invoked. Before just the building of the command was encapsulated and they were invoked in each E2E test. Encapsulating them both and organizing them into a class makes it more consistent and easier to change later because there is only one place to change instead of 3+ places.
  • Classes support inheritance. So I can create another more generic TestRunner classes for CLI invocation. This class can inherit overlapping logic from that.
  • Some of the E2E test logic was spread out over multiple files and the class groups the related logic together. So similar to point 1. Easier to find/change and manage as the test suite grows.

@jpower432
Copy link
Member Author

@JimFuller-RedHat PTAL. Relates to a comment you had on #158.

@jpower432 jpower432 merged commit 49ff3b8 into main Mar 5, 2024
11 checks passed
@jpower432 jpower432 deleted the chore/e2e-refactor branch March 6, 2024 21:36
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.

Update e2e test code to encapsulate the building and running process for commands
2 participants