Skip to content

Feature/#1019 #1083

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
May 19, 2023
Merged

Feature/#1019 #1083

merged 4 commits into from
May 19, 2023

Conversation

linkdotnet
Copy link
Collaborator

This PR closes #1019

Additionally I did the following:

  • Made BunitTestRenderer internal

@linkdotnet linkdotnet requested a review from egil May 18, 2023 20:26
@linkdotnet linkdotnet changed the base branch from main to v2 May 18, 2023 20:27
@linkdotnet linkdotnet force-pushed the feature/#1019 branch 2 times, most recently from 84adb72 to a22812f Compare May 19, 2023 06:27
@egil
Copy link
Member

egil commented May 19, 2023

I do not think there is a reason to keep ITestRenderer around. Lets remove that at the same time.

Why do you think we should make BunitRenderer internal?

@linkdotnet
Copy link
Collaborator Author

I do not think there is a reason to keep ITestRenderer around. Lets remove that at the same time.

Why do you think we should make BunitRenderer internal?

Originally I made it internal because there was no need to have the interface and implementation.
I am much in favor of not exposing any internal stuff to the public for some reasons:

  1. Flexibility: We can just change the renderer without worrying.
  2. Control over use: I am looking towards the deterministic renderer where the user could control render cycles outside of the WaitFor helpers. As the API is there probably users will run into that pitfall. We can protect them from doing such things.
  3. The renderer is an implementation detail: we need it to create something the user can make easy asserts on - but how and why this is done is our responsibility.

If we abolish the interface, probably we have to make the renderer public or remove some functions that are present on the renderer itself. All in all I don't see much in favor of having the BunitRenderer available for a user.

@egil
Copy link
Member

egil commented May 19, 2023

I did a quick search on GitHub, and there are a some that uses TestRenderer in their code, so I think it is worthwhile to keep that public, with some/most of our methods marked internal. But it would be great if we just call it BunitRenderer and skip the interface.

@linkdotnet
Copy link
Collaborator Author

I did a quick search on GitHub, and there are a some that uses TestRenderer in their code, so I think it is worthwhile to keep that public, with some/most of our methods marked internal. But it would be great if we just call it BunitRenderer and skip the interface.

Hmm isn't that the interface version ITestRenderer? So in conclusion: Abolish the interface but keep TestRenderer public, did I get this correctly?

@egil
Copy link
Member

egil commented May 19, 2023

I did a quick search on GitHub, and there are a some that uses TestRenderer in their code, so I think it is worthwhile to keep that public, with some/most of our methods marked internal. But it would be great if we just call it BunitRenderer and skip the interface.

Hmm isn't that the interface version ITestRenderer? So in conclusion: Abolish the interface but keep TestRenderer public, did I get this correctly?

When I searched for "TestRenderer" I found too many hits from other frameworks that uses a similar name. "ITestRenderer" is a bit more specific, and "BunitRenderer" will be even more searchable :)

egil
egil previously approved these changes May 19, 2023
@linkdotnet linkdotnet merged commit 7343672 into v2 May 19, 2023
@linkdotnet linkdotnet deleted the feature/#1019 branch May 19, 2023 19:34
egil pushed a commit that referenced this pull request Jun 10, 2023
* refactor: Rename TestRenderer to BunitRenderer

* refactor: Remove ITestRenderer in favor of BunitRenderer

* Small fixup
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename TestRenderer to BunitRenderer
2 participants