Add 77 IntegrationTests and lots of fixes

[softworkz]

• StartupManager: Add support for running under testhost
• BrowserView.cs: Fix cast exception in Bounds property getter
• App.cs: Fix UserAgentFallbackAsync
• ipc.ts: Add helper method for tests
• browserWindows.ts: Fix SetThumbarButtons
• browserWindows.ts: Add catch for Set/GetRepresentedFilename...
• notification.ts: Fix notificationIsSupported...
• webContents.ts: Fix clearAuthCache invocation...
• main.js: Load api/process import (was missing)
• ProcessMetric: Fix deserialization error for CreationTime
• ApiBase: fix event names for App
• BrowserWindow: Disable SetPosition 'workaround'...
• browserWindowSetParentWindow: Support null parameter
• Add IntegrationTests project

[Copilot]

Pull Request Overview

This PR adds comprehensive integration tests for the ElectronNET project and fixes several bugs discovered during test development. Key changes include:

• A new integration test project with extensive test coverage for BrowserWindow, WebContents, App, IPC, and other Electron APIs
• Bug fixes for API compatibility issues (Notification.isSupported, parent window handling, represented filename)
• Improvements to socket event naming consistency and property getter implementation
• Enhanced error handling for platform-specific features
• Process metric type correction from int to double for CreationTime

Reviewed Changes

Copilot reviewed 40 out of 44 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
ElectronNET.sln	Adds integration test project to solution
ElectronNET.IntegrationTests/*.cs	Integration tests covering major Electron APIs
ElectronFixture.cs	Test fixture for shared Electron runtime
notification.ts/js	Fixes isSupported to call as function
browserWindows.ts/js	Adds error handling for platform-specific APIs and thumbar button fixes
ipc.ts/js	Adds integration test helpers for menu item clicking
webContents.ts/js	Adds clearAuthCache overload support
StartupManager.cs	Adds testhost detection for assembly resolution
ProcessMetric.cs	Changes CreationTime from int to double
BrowserWindow.cs	Fixes SetParentWindow null handling and removes Windows 10 position workaround
ApiBase.cs	Refactors socket event naming to support multiple patterns
App.cs	Fixes UserAgentFallbackAsync property implementation
BrowserView.cs	Improves async Bounds property getter

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[softworkz]

Note that two tests are failing because of this: #905 (comment)

[FlorianRappl]

That is much appreciated - and I'm happy that it already confirms two issues. Only question is - should we fix them in this PR or make a subsequent PR? As it stands we need to have a baseline with all tests working - otherwise future PRs will always have these two failing tests.

[softworkz]

That is much appreciated - and I'm happy that it already confirms two issues. Only question is - should we fix them in this PR or make a subsequent PR? As it stands we need to have a baseline with all tests working - otherwise future PRs will always have these two failing tests.

This is a bigger change - not for this PR, I'd say.

I'm waiting for a comment from @agracio as I thought it wouildn't be nice to submit a PR which fundamentally changes his contribution, so we should wait for whether he wants to work on fixing it.

These options exist:

• @agracio submits a fix
• I create a fix
• We revert @agracio's PR
  (temporarily)

We could do the 3rd one either way, this should fix it.

As it stands we need to have a baseline with all tests working - otherwise future PRs will always have these two failing tests.

As long as Ninja Nuke doesn't run tests, it's probably not a big thing when the two tests are failing for a short timespan, no?
We could also set the tests to be skipped like [Fact(Skip = "Currently faiiling, fix pening")]

[agracio]

I was under the impression that Action variables are passed by reference rather than value.

EDIT: now that I think about it they are always passed as null first so there is no such thing as reference here.

Look like I have made a mistake there.
Can we confirm that those tests are running fine without my PR.

Not sure how to fix this particular issue in the context of the code I created.

[softworkz]

I was under the impression that Action variables are passed by reference rather than value. Look like I have made a mistake there. Can we confirm that those tests are running fine without my PR.

I've seen the issue from the code alone before even trying, so I'm pretty sure that it's like that. But you can surely check out this branch and revert your commits on top. Or you don't even have to do that actually, you just need to revert the two events being tested to the older code. That way you won't have any conflicts.

Btw, there are more issues in the EventHelper which I haven't mentioend: At two or more places, you do callback+= value only inside the if statement bodies.

I was under the impression that Action variables are passed by reference rather than value

Yup, that's total unintuitive - if I wouldn't have known it, I would have made the same assumption.
It's actually possible to pass it by reference with ref, but then you cannot use it in the closure of the event callback...

[softworkz]

EDIT: now that I think about it they are always passed as null first so there is no such thing as reference here.

Multicast delegates are a very special kind of animal in C# - even the null check is kind-of fake...

[softworkz]

Not sure how to fix this particular issue in the context of the code I created.

Did you read my comments in your PR?

[agracio]

So even if I remove closures and pass them by ref it will not make a difference, since they are null when they are initially passed I do not believe it will resolve the issue.

[agracio]

Creating a dictionary of delegates, yes that would be possible.
I have also tested CallerMemberName on events and it does work even though the call comes from add or remove.

[softworkz]

So even if I remove closures and pass them by ref it will not make a difference, since they are null when they are initially passed I do not believe it will resolve the issue.

When you pass them by ref, the receiver is added like when you'd do it on the calling side - i.e. second time it won't be null.
But you won't be able to get it stored for the callback to use. And even when you manage to get it into the closure, then it will be byvalue and subsequently added or removed receivers won't change the delegate that you have passed to the event callback.

[softworkz]

Creating a dictionary of delegates, yes that would be possible. I have also tested CallerMemberName on events and it does work even though the call comes from add or remove.

Yes - same like in case of properties.

Creating a dictionary of delegates, yes that would be possible.

Another weirdness: Action and Action<T> do not have a base class, but both can be casted to delegate.
This means you don't need separate dictionaries for the different event signatures.

[agracio]

Another weirdness: Action and Action do not have a base class, but both can be casted to delegate.
This means you don't need separate dictionaries for the different event signatures.

I do not think it is needed in the first place, only one dictionary would store eventName->Action.
I do not think that events require to track both names since we are not using Off().
There is no need to have separate dictionaries for On() and Emit().

[agracio]

You PR should be merged first in my opinion, much easier to work for both of us when there are no competing PRs in the same branch that might be touching the same code.

[softworkz]

I do not think it is needed in the first place, only one dictionary would store eventName->Action.

You cannot cast Action<T> to Acttion or vice versa, that's what I meant.

[agracio]

As for the rest of the comment you left on my PR: my intention was never to change the existing code - I refactored it as is including string concatenations, no null checks etc...

[softworkz]

As for the rest of the comment you left on my PR: my intention was never to change the existing code - I refactored it as is including string concatenations, no null checks etc...

Which comment do you mean..?

[agracio]

I do not think it is needed in the first place, only one dictionary would store eventName->Action.

You cannot cast Action to Acttion or vice versa, that's what I meant.

A very good point forgot about it.

[softworkz]

A very good point forgot about it.

That's why I said that both can be cast to delegate - so you can make a single dictionary of eventName->delegate and cast the delegate to Action/ActionT as needed. 😁

[agracio]

Which comment do you mean..?

Sorry meant to say comments - plural

• Id that should be int but it can also be string.Empty for events without Id or I can change it to null
• int.ToString(CultureInfo.InvariantCulture) conversion
• callback invocation needs null-check - like callback?.Invoke()

As I said the code was refactored the way it was used before, no improvements or changes of any kind and was never my intention.

[agracio]

Btw having integration tests in Electron.NET is a huge milestone.

[softworkz]

• Id that should be int but it can also be string.Empty for events without Id or I can change it to null

Yea, make it nullable

As I said the code was refactored the way it was used before, no improvements or changes of any kind and was never my intention.

Ah, now I got it - I didn't look how it has s been before, so it's been a (tiny) bit unfair, but it had been merged already, so I didn't think in terms of what would be a blocker and what not.
Anyway, it's not about criticism, it's about collaborative improvement.

[agracio]

See my comment #913 (comment) - I believe this PR should be merged first, as my changes might also touch on so ts/js files and definitely on files that are changed in this RP.

Would like to avoid merge complications.

[softworkz]

See my comment #913 (comment) - I believe this PR should be merged first, as my changes might also touch on so ts/js files and definitely on files that are changed in this RP.

Would like to avoid merge complications.

Agreed. But you can check out this PR and start working on top of it already (if you like)..

[agracio]

Agreed. But you can check out this PR and start working on top of it already (if you like)..

I am already working on a prototype.

On a completely off-topic note - every time I open solution in Rider it tells me that Nuke.Common 8.0.0 contains vulnerabilities.
Would be a good idea to upgrade to a higher version.

[FlorianRappl]

As long as Nuke doesn't run tests, it's probably not a big thing when the two tests are failing for a short timespan, no?

Definitely. My comment was rather meant as a "we should integrate it into NUKE / the build system ASAP then ...". But then my suggestion is to merge (just did) and integrate once we have all tests succeeding. Until then we can just run tests locally to verify if everything (except the known ones) is still running.

Huge milestone!

Many thanks.

On a sidenote: I should get a chatroom for the two of you @agracio @softworkz :D. Not the conversation is not meaningful (I like it), but definitely a lot to digest before getting into the relevant pieces here. Chatroom may be a good trigger point as right now gitter is still used (but last time I checked the room was inactive & I know that at least @GregorBiswanger and @FlorianRappl are only active on Discord any more - so maybe let's move there, too?).

[agracio]

@softworkz are there any steps that need to be done to run integration tests, they all fail on my PC.

        private void SetElectronExecutable()
        {
            string executable = ElectronNetRuntime.BuildInfo.ElectronExecutable;
            if (string.IsNullOrEmpty(executable))
            {
                throw new Exception("AssemblyMetadataAttribute 'ElectronExecutable' could not be found!");
            }

            if (Environment.OSVersion.Platform == PlatformID.Win32NT)
            {
                executable += ".exe";
            }

            ElectronNetRuntime.ElectronExecutable = executable;
        }

string executable = ElectronNetRuntime.BuildInfo.ElectronExecutable; is always null and throws "AssemblyMetadataAttribute 'ElectronExecutable' could not be found!"

[softworkz]

Definitely. My comment was rather meant as a "we should integrate it into NUKE / the build system ASAP then ...". But then my suggestion is to merge (just did) and integrate once we have all tests succeeding. Until then we can just run tests locally to verify if everything (except the known ones) is still running.

👍

On a sidenote: I should get a chatroom for the two of you @agracio @softworkz :D. Not the conversation is not meaningful (I like it), but definitely a lot to digest before getting into the relevant pieces here. Chatroom may be a good trigger point as right now gitter is still used (but last time I checked the room was inactive & I know that at least @GregorBiswanger and @FlorianRappl are only active on Discord any more - so maybe let's move there, too?).

It won't happen regularly, I don't have that much time, but I also hate to be the one having to say that it's all wrong after submission (and effort by a contributor), so I rather tell beforehand when I know the caveats already.
Regarding chat: I have accounts for everything, I even registered for Gitter. I'll happily log in somewhere on request, but I haven't running any of these regularly (can't deal with the distraction...), so I don't mind whatever it is...

[softworkz]

string executable = ElectronNetRuntime.BuildInfo.ElectronExecutable; is always null and throws "AssemblyMetadataAttribute 'ElectronExecutable' could not be found!"

Look for these lines in StartupInfo:

            if (electronAssembly.GetName().Name == "testhost")
            {
                electronAssembly = AppDomain.CurrentDomain.GetData("ElectronTestAssembly") as Assembly ?? electronAssembly;
            }

If you're not using Visual Studio, your test runner might not be "testhost". Check out what it is and add it to that condition.

[agracio]

It looks like fix is not as straightforward as I initially thought. Even when using dictionary test fails but with a different result.

Here is a very basic code change.

private static readonly ConcurrentDictionary<string, Delegate> events = new();

internal static void AddEvent(string eventName, object id, Action callback, Action value)
{
    var call = (Action)events.GetOrAdd(eventName, callback);
    if (call == null)
    {
        BridgeConnector.Socket.On(eventName + id, () => { call(); });
        BridgeConnector.Socket.Emit($"register-{eventName}", id); 
    }
    call += value;
    events[eventName] = call;
}

Checking in debugger it shows that all 3 listeners are subscribed co call by the end of the 3 calls.

h1.TrySetResult();
h2.TrySetResult();
h3.TrySetResult();

But only the first event is successful

Not all events were fired: 
Event1 fired: True
Event2 fired: False
Event3 fired: False

Pretty sure that I am missing something completely obvious.

[agracio]

Figured out the problem

internal static void AddEvent(string eventName, object id, Action callback, Action value)
{
    var call = (Action)events.GetOrAdd(eventName, callback);
    if (call == null)
    {
        BridgeConnector.Socket.On(eventName + id, () =>
        {
            ((Action)events[eventName])();
        });
        BridgeConnector.Socket.Emit($"register-{eventName}", id);
    }
    
    call += value;
    events[eventName] = call;
}

@softworkz could you advise whether it requires any locks and if so where.

[agracio]

I am moving events into ApiBase and will follow naming pattern using CallerMemberName, this makes the most sense to me now.
Should have something ready either later today or tomorrow.

[softworkz]

@softworkz could you advise whether it requires any locks and if so where.

@agracio - please see here: #918 - even though you got it working somehow, my earlier advice was a bit too simplistic, I'm sorry about that. As a remedy, I created a solid pattern that you can use.