Turn on the CI build for PRs

[gdziadkiewicz]

Fixes #106

TODO:

• Confirm that tests are really being executed on each of the platforms with expected frameworks
• Confirm how the tests results look like for failing build and failing tests and document
• (optional) Introduce caching for fixed part (npm build deps, nuget test deps?)

[gdziadkiewicz]

Local run in my fork: https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11637652589

[FreeAndNil]

@gdziadkiewicz can you test how just using "dotnet test" as a single step would look like?
@fluffynuts do you have an idea why the windows output is much longer? (the macos and linux output are missing important inforamtion)

[gdziadkiewicz]

@gdziadkiewicz can you test how just using "dotnet test" as a single step would look like? @fluffynuts do you have an idea why the windows output is much longer? (the macos and linux output are missing important inforamtion)

Will do.

[gdziadkiewicz]

Here is the log from the dotnet test run https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11660840781/job/32463975074

[fluffynuts]

minor: build step is not required - npm test will build as a pre-requisite

[gdziadkiewicz]

That's true, but it's easier to tell what failed when the phases are separated.

[fluffynuts]

@gdziadkiewicz can you test how just using "dotnet test" as a single step would look like? @fluffynuts do you have an idea why the windows output is much longer? (the macos and linux output are missing important inforamtion)

Will do.

the windows output looks like the output from npm test where the other two look like the output from dotnet test - the default verbosity on dotnet test will only dump out failing tests.

I'd like to suggest that you install the Quackers test logger (https://www.nuget.org/packages/quackers.testlogger) - in conjunction with zarro, you'll get easier-to-read output (colored, with pass/fail icons) and it will suppress duplicate logging from the dotnet logger. This mechanism also allows for faster testing by running test assemblies in parallel, if log4net is ever looking at having a second tests assembly (or looking to reduce overall test-time: split existing tests across multiple test assemblies, as long as they all have a name ending in .Tests. Parallel testing is a fairly large reason why I use this approach everywhere.

Alternatively, if y'all really want to eschew npm test, I suggest raising the verbosity for dotnet test - but I still heartily recommend the zarro / quackers route (both are written by me, and are our defaults at work) as you'll get the best output that way (eg: see https://github.com/fluffynuts/NExpect/actions/runs/11629746737/job/32387287891 for an example)

[FreeAndNil]

@fluffynuts we already use Quackers test logger in log4net.Tests.

[FreeAndNil]

Here is the log from the dotnet test run https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11660840781/job/32463975074

@gdziadkiewicz dotnet test brings the full output on all platforms.

[FreeAndNil]

Can you remove this step since dotnet test also builds the DLLs?

[gdziadkiewicz]

I would rather not do that. With separate steps, it's easy to see without digging into whether it's a build issue or a failing tests issue.

[FreeAndNil]

Good point.

[gdziadkiewicz]

@gdziadkiewicz can you test how just using "dotnet test" as a single step would look like? @fluffynuts do you have an idea why the windows output is much longer? (the macos and linux output are missing important inforamtion)

Will do.

the windows output looks like the output from npm test where the other two look like the output from dotnet test - the default verbosity on dotnet test will only dump out failing tests.

I'd like to suggest that you install the Quackers test logger (https://www.nuget.org/packages/quackers.testlogger) - in conjunction with zarro, you'll get easier-to-read output (colored, with pass/fail icons) and it will suppress duplicate logging from the dotnet logger. This mechanism also allows for faster testing by running test assemblies in parallel, if log4net is ever looking at having a second tests assembly (or looking to reduce overall test-time: split existing tests across multiple test assemblies, as long as they all have a name ending in .Tests. Parallel testing is a fairly large reason why I use this approach everywhere.

Alternatively, if y'all really want to eschew npm test, I suggest raising the verbosity for dotnet test - but I still heartily recommend the zarro / quackers route (both are written by me, and are our defaults at work) as you'll get the best output that way (eg: see https://github.com/fluffynuts/NExpect/actions/runs/11629746737/job/32387287891 for an example)

WIP

[gdziadkiewicz]

Hi all, I amended the csproj to use quackers by default, and it looks great (I'm a new fan!). As for Zarro, at the moment, the builds are taking longer with it than without it (especially for Windows, it's 2x longer), and there is this surprising requirement of having node/npm to build a dotnet project. Would going with Quackers and dropping Zarro be acceptable? Open to discuss @fluffynuts @FreeAndNil

P.S. From now on, I will respond much more quickly.

[FreeAndNil]

I would like to skip zarro at this point and add it later in case we need it.
It was the best approach when it was added, because the dotnet tool could not support our use case and the legacy frameworks.
But now it does all we need.

[fluffynuts]

Hi all, I amended the csproj to use quackers by default, and it looks great (I'm a new fan!). As for Zarro, at the moment, the builds are taking longer with it than without it (especially for Windows, it's 2x longer), and there is this surprising requirement of having node/npm to build a dotnet project. Would going with Quackers and dropping Zarro be acceptable? Open to discuss @fluffynuts @FreeAndNil

P.S. From now on, I will respond much more quickly.

The only problem that I can think of with using quackers without zarro is that the dotnet test system has a bug (I opened an issue, and they took months to get back to me, and basically said "yeah, we know, but we're re-working all of the test internals, so we're closing this") where it doesn't suppress all console logging, even when explicitly told to only use quackers - in particular, you'll see failures twice, though the final stats and all output by quackers will be the same.

Zarro contains hacks to suppress re-echoing these outputs, by instructing quackers to prepend all log lines with an obscure string sequence that it can use to decide if a log line comes from quackers or not.

I also expect that the longer build time via zarro is largely due to the legacy behavior of splitting nuget restore and build - msbuild didn't use to restore packages on build, where it does now. I should probably look into that at some point, but builds have been adequately fast for all the use-cases I've encountered, so it hasn't been high priority.

[FreeAndNil]

@fluffynuts are you OK with skipping zarro in the pipeline for now?

[fluffynuts]

@fluffynuts are you OK with skipping zarro in the pipeline for now?

Sure, if you want to. Zarro does a lot of things and a lot of them are not required for log4net. I know the node/npm requirement can be confusing for some - part of the aim is standardizing how we (me,my work, etc) build and test things, ie via npm run build and npm test (and zarro makes extension of the tasking system really easy) instead of having to read documents on how to build, etc. For CI, I guess it doesn't matter much.

[FreeAndNil]

@gdziadkiewicz LGTM.
Can you make a test fail so that we can see that the pipeline fails?

[gdziadkiewicz]

Here is a build error https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11842227538/job/33000348058?pr=1 and here is a failing test https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11845311348/job/33010485591?pr=1

[fluffynuts]

Here is a build error https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11842227538/job/33000348058?pr=1 and here is a failing test https://github.com/gdziadkiewicz/logging-log4net/actions/runs/11845311348/job/33010485591?pr=1

the output from quackers is looking better than i expected without zarro (:

[FreeAndNil]

Nice, thanks a lot.