Skip to content

Conversation

@mxschmitt
Copy link
Contributor

@mxschmitt mxschmitt commented Nov 8, 2024

First stab for adding xUnit testing harness for Playwright since they changed their parallelism algorithm which unblocked us. So far I found the following questions along the way:

  • Microsoft.Playwright.xUnit vs Microsoft.Playwright.XUnit
  • Is it normal that Console.WriteLine doesn't work in the test method? I had to follow this guide instead to make it work. Looks like we can use Console / Trace / Debugger capture support xunit/xunit#1730 (comment) once its released.
  • Do we stick with AppDomain.CurrentDomain.FirstChanceException to determine if a test failed and restart the worker services (mostly Browser) appropriately? Inspired by @SimonCropp from here. We need some hack since there is no TestContext in xUnit.
  • Is targeting net8.0 fine? AppDomain is only available in net5.0+. xUnit itself targets net472;net6.0. Maybe we should target both as well and provide AppDomain support only for the net6.0 output.

Curious about your inputs @kblok @campersau @nohwnd thank you in advance!

Relates #2977

@campersau
Copy link
Contributor

Is it normal that Console.WriteLine doesn't work in the test method?

Yes.

Do we stick with AppDomain.CurrentDomain.FirstChanceException to determine if a test failed and restart the worker services appropriately?

If you wait a little xunit.v3 will be released and it contains new features like a TestContext and CaptureConsoleAttribute which might help.
See

@kblok
Copy link
Contributor

kblok commented Nov 11, 2024

  • Microsoft.Playwright.xUnit vs Microsoft.Playwright.XUnit

Maybe we could follow their name xunit, Microsoft.Playwright.xunit. Not that pretty. Microsoft.Playwright.XUnit would be my second choice.

Yes. That was a headache. We were using XunitLoggerProvider in the old days.

@nohwnd
Copy link
Member

nohwnd commented Nov 20, 2024

I don't have any comments to the code, it all looks pretty good / standard to me.

I would name the namespace, Microsoft.Playwright.Xunit, because it is the least special, and also how they capitalize their namespace / class. https://github.com/xunit/xunit/blob/main/src/xunit.v3.runner.utility/Frameworks/v2/Xunit2.cs#L15

And the nuget Microsoft.Playwright.XUnit. Because I see the capitalization with existing packages all over the place.

@bradwilson do you have different preferences on the capitalization of the xunit name?

@bradwilson
Copy link

In theory, the only capitalization "rules" are:

  • Product name = xUnit.net
  • Class name or namespace name = Xunit

In practice, people do whatever they want. 😁

The product name being "xUnit.net" is really because back in the day, "xUnit" meant "replace 'x' with something that represents your programming environment". SUnit = Smalltalk, JUnit = Java, NUnit = .NET. Since NUnit was already taken, we opted for "xUnit.net" as the next best name. (And by "we" I mean @jnewkirk who also was part of creating NUnit. 😉)

File names or package names, we don't care about. We use all lowercase for ours, but we've never asked anybody to follow that pattern. Frankly it's like that mostly because I'm an Extra Oldster™️ who likes lowercase file names because of case sensitive file systems.

@bradwilson
Copy link

I will say, XUnit looks wrong to my eyes, but that's a bit of personal preference. I think for package names Xunit would be better than XUnit; or if you feel strongly about capitalizing the U, then use xUnit. We never capitalize the X and U together.

@nohwnd
Copy link
Member

nohwnd commented Nov 20, 2024

I don’t feel strongly about the name, but I do prefer I prefer Xunit over xUnit in the package name.

return await browserType.LaunchAsync(PlaywrightSettingsProvider.LaunchOptions).ConfigureAwait(false);
}

var exposeNetwork = Environment.GetEnvironmentVariable("PLAYWRIGHT_SERVICE_EXPOSE_NETWORK") ?? "<loopback>";
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all this service code right away? If so, can it at least be shared with other runners?

@mxschmitt mxschmitt merged commit 8cd88de into main Nov 21, 2024
12 of 15 checks passed
@mxschmitt mxschmitt deleted the feature/xunit branch November 21, 2024 23: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.

7 participants