Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Improve Tests #301

Open
TrabacchinLuigi opened this issue Mar 15, 2022 · 8 comments
Open

Improve Tests #301

TrabacchinLuigi opened this issue Mar 15, 2022 · 8 comments

Comments

@TrabacchinLuigi
Copy link
Contributor

TrabacchinLuigi commented Mar 15, 2022

Unit tests seem to be working as integration tests: mounting a disk and using that disk.
Because of that moq is being used to verify calls from the driver instead of creating mocks to be fed to the system under test.
Also, it is impossible for now to use moq with Spans devlooped/moq#1049 (and i belive we should not be held back by that)

I think we should try to use the marshal class to tests if the class / struct we wrote can be marshalled https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.copy

@LTRData
Copy link
Contributor

LTRData commented Mar 15, 2022

Not sure really what you are trying to do here. Structures can be directly pinned and used by unmanaged code through pinned pointer if the structure is blittable. You can test that using the unmanaged type constraint on a generic type definition for example. Other kind of structs need marshalling. So effectively, if the struct in unmanaged in this way we could instead use MemoryMarshal class to directly get references to elements of arrays, copy from and to unmanaged memory etc.
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.memorymarshal

@TrabacchinLuigi
Copy link
Contributor Author

To be honest i haven't already been through all the tests, so i'm not 100% sure of what need to be done.
My goal is to keep the spans in my fork, and propose also some tests to ensure no mistakes where made which the compiler can't check (for instance when I've changed the DokanFileInfo into a struct it was compiling right away, but during runtime it was complaining about the no more valid attribute MarshalAs(UnmanagedType.LPStruct)

Also now i'm receiving contexts that seem to have lost their context, probably because i need to pass them down by reference, but the compiler won't say anything

@LTRData
Copy link
Contributor

LTRData commented Mar 15, 2022

Where did you get the error about MarshalAs attribute? I think you could compare your code to my fork. I turned DokanFileInfo to a struct some time ago and it has been used a lot in some applications since that change. I just checked in latest changes with some code cleanups such as auto-properties etc.
https://github.com/LTRData/dokan-dotnet/blob/LTRData.DokanNet-initial/DokanNet/DokanFileInfo.cs

@TrabacchinLuigi
Copy link
Contributor Author

Yes, i went to have a look, it was on the DokanOperationProxy, i'm trying not to watch too often or i won't understand what i'm doing 🤣 i end up doing the same thing, remove the marshalas attribute at all

@LTRData
Copy link
Contributor

LTRData commented Mar 15, 2022

Okay, MarshalAs attribute is really only needed for selecting a specific method for marshalling. There is no reason to use that attribute when passing blittable structures by reference since that is by default done simply using a pointer to the structure instance, without any marshalling or memory copies.

@TrabacchinLuigi
Copy link
Contributor Author

TrabacchinLuigi commented Mar 15, 2022

I've figured out I can do something like this (the examples are for my branch or @LTRData branches which uses DokanFileInfo as a structure)

[TestClass]
public class MarshallingTests
{
    [TestMethod]
    public void DokanOperations_should_be_marshallable()
    {
        var sut = new DOKAN_OPERATIONS();
        _ = new NativeStructWrapper<DOKAN_OPERATIONS>(sut);
    }

    [TestMethod]
    public void DOKAN_OPERATIONS_ZwCreateFile_should_be_marshallable()
    {
        static NtStatus CreateFile(
            string rawFileName,
            IntPtr securityContext,
            uint rawDesiredAccess,
            uint rawFileAttributes,
            uint rawShareAccess,
            uint rawCreateDisposition,
            uint rawCreateOptions,
            ref DokanFileInfo dokanFileInfo) { return NtStatus.Unsuccessful; }

        var sut = new DOKAN_OPERATIONS()
        {
            ZwCreateFile = new DokanOperationProxy.ZwCreateFileDelegate(CreateFile)
        };
        _ = new NativeStructWrapper<DOKAN_OPERATIONS>(sut);
    }

    [TestMethod]
    public void DOKAN_OPERATIONS_CleanUp_should_be_marshallable()
    {
        static void CleanUp(string rawFileName, ref DokanFileInfo rawFileInfo) { }

        var sut = new DOKAN_OPERATIONS()
        {
            Cleanup = new DokanOperationProxy.CleanupDelegate(CleanUp)
        };
        _ = new NativeStructWrapper<DOKAN_OPERATIONS>(sut);
    }
}

Then if i go to the delegates and sabotage them like this

public delegate NtStatus ZwCreateFileDelegate(
  [MarshalAs(UnmanagedType.LPWStr)] string rawFileName,
  IntPtr securityContext,
  uint rawDesiredAccess,
  uint rawFileAttributes,
  uint rawShareAccess,
  uint rawCreateDisposition,
  uint rawCreateOptions,
  [MarshalAs(UnmanagedType.LPStruct), In, Out] ref DokanFileInfo dokanFileInfo);

The second test method will fail

@TrabacchinLuigi
Copy link
Contributor Author

@Liryna I've updated my branch, what do you think we need to be tested ? i could move the integration tests to a dedicated project, but i feel this is a more sensible way to tests what dokan.net is doing, Dokany should have it's own tests

@Liryna
Copy link
Member

Liryna commented Mar 17, 2022

So the Mock test looks to be a good idea but in reality it is way harder to maintain it (env changes that issue new IO from a third party make the test fail) than it actually detect issues.

We have tools that generates IO like winfstest that are more adapted https://github.com/dokan-dev/dokan-dotnet/blob/master/appveyor.yml#L91
We are missing fsx https://github.com/dokan-dev/dokany/blob/master/samples/memfs_test.ps1#L151-L153 which should be added.

What I suggest is that dokan-dotnet test ensure the code outside the simple native code wrapping be tested. Like the BufferPool / DokanHelper ...
If we have a native wrapping issue, we should have the previous tools detect it. Hope that make sense.

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

No branches or pull requests

3 participants