Skip to content

Conversation

@google-labs-jules
Copy link
Contributor

This change refactors the FUnit class to use a nested TestSuite class. This improves the structure of the code by separating the test execution logic from the main FUnit class.


PR created automatically by Jules for task 4991649363410170469

This refactoring addresses the TODO in FUnit.cs by introducing a nested `TestSuite` class to encapsulate test aggregation and execution logic.

Key changes:
- The `FUnit` class is now a `partial` class.
- A new nested class `TestSuite` is created within `FUnit`.
- Test aggregation logic is moved to the `TestSuite` constructor.
- Test execution logic is moved to the `TestSuite.ExecuteAsync` method.
- A `Build` method is added to `FUnit` to create `TestSuite` instances.
- `FUnit.RunAsync` is updated to use the `TestSuite` class and handles logging via events.
- Events (`TestRunStarting`, `SubjectStarting`, `TestCaseSkipped`) are introduced in `TestSuite` to decouple execution from logging.
@google-labs-jules
Copy link
Contributor Author

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll acknowledge your comments with a 👀 emoji and then get to work. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

I will automatically address your feedback. For any comments you don't want me to act on, just include (aside).



For security, I will only act on instructions from the user who triggered this task for this pull request.

@sator-imaging sator-imaging self-requested a review September 21, 2025 08:32
@sator-imaging sator-imaging marked this pull request as ready for review September 21, 2025 08:32
The `TestCaseSkipped` event in the `TestSuite` class was exposing the internal `TestCase` type in its public API.

This commit changes the event signature to use a tuple `(string subject, string description)` instead, which provides the necessary information without exposing internal implementation details. The event handler in `FUnit.RunAsync` has been updated accordingly.
}
}

public async Task<(List<FailedTestCase> failed, List<TestCase> skipped)> ExecuteAsync(
Copy link
Owner

Choose a reason for hiding this comment

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

it should return TestResult instead. change TestResult.Test to have TestResult.Error list. (current: Error?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've refactored TestSuite.ExecuteAsync to return a TestResult object directly. I've also updated TestResult.Test to hold a list of errors as you suggested.

Copy link
Owner

Choose a reason for hiding this comment

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

so many changes applied, revert it to previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted the changes as you requested. The code is now back to the previous state.

This commit refactors `TestSuite.ExecuteAsync` to construct and return the `TestResult` object directly, instead of a tuple of failed and skipped tests. This simplifies the `FUnit.RunAsync` method and centralizes the test result creation logic within the `TestSuite`.

Additionally, the `TestResult.Test` record has been updated to hold a list of errors (`IReadOnlyList<Error>`) instead of a single nullable error. This allows for capturing multiple errors for a single test case, which might be useful for features like running tests multiple times.
This reverts the previous commit that refactored `TestSuite.ExecuteAsync` to return a `TestResult` object. The change was deemed too extensive and has been reverted as per user feedback.
@sator-imaging sator-imaging merged commit b41e626 into main Sep 21, 2025
@sator-imaging sator-imaging deleted the refactor-funit-testsuite branch September 24, 2025 01:23
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.

1 participant