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

Added missing tests to kernel32 and DbgHelp. #249

Closed
wants to merge 1 commit into from

Conversation

AraHaan
Copy link
Contributor

@AraHaan AraHaan commented Apr 28, 2021

Fixes #224

(Tests created on my Mac)

@AraHaan AraHaan force-pushed the added-missing-tests branch from 26faf0d to 9c2fc46 Compare April 28, 2021 06:30
@AraHaan
Copy link
Contributor Author

AraHaan commented Apr 28, 2021

Wait why doesn't the pipeline test ns2.0 as well so it can expose the failures on these when targeting that TFM?

@AArnott
Copy link
Member

AArnott commented May 20, 2021

why doesn't the pipeline test ns2.0 as well

netstandard2.0 isn't a runtime. It's just a standard. Tests can only run on a runtime like .NET Core, .NET Framework, or mono.
It looks like we only run tests on .NET 5, regardless of OS. For the code generation tests that seems adequate since I do not expect the runtime to influence the code generation itself. But we could add more runtimes if necessary.

@AArnott
Copy link
Member

AArnott commented May 20, 2021

We don't want a test for every API to make sure it generated. We have tests for specific APIs that tend to be problematic and throw exceptions during generation, but ultimately those are redundant with our "generate everything" tests. Why do we need these new tests?

@AraHaan
Copy link
Contributor Author

AraHaan commented May 20, 2021

Because a lot of those api's I added are technically some of the common ones I find that people want to use on the internet, myself included.

Sure, one could use System.Diagnostics.Process but what if that is not provided but those APIs are?

Plus there is an issue where the ProcessId returned from that class (from the Process.GetCurrentProcess() call would not return the native ProcessId and instead the "managed" one when needing to call an unmanaged api and then as that is concerned it is according to the OS is not a valid id or is an id that points to an entirely different process.

I guess I could remove 3 of the tests but add them to the interesting apis.

  • GetCurrentProcess
  • GetCurrentProcessId
  • GetCurrentThreadId

And have MiniDumpWriteDump and MINIDUMP_TYPE be their own tests.

@AraHaan AraHaan force-pushed the added-missing-tests branch from 9c2fc46 to efe91d6 Compare May 20, 2021 22:41
@AraHaan
Copy link
Contributor Author

AraHaan commented May 20, 2021

I see this is why I have my issue on my codebase not generating this enum:

  Failed GeneratorTests.GetMINIDUMP_TYPEGeneration [126 ms]
  Error Message:
   Assert.True() Failure
Expected: True
Actual:   False
  Stack Trace:
     at GeneratorTests.GetMINIDUMP_TYPEGeneration() in D:\a\1\s\test\Microsoft.Windows.CsWin32.Tests\GeneratorTests.cs:line 248

Which is odd because it should be constant across all cpu specific builds of Windows 10 that has the latest dbghelp.dll.

I think this should be rebased on top of #272 after that gets merged and see if that test starts to pass but change the other test to somehow test it being generated when it is set up correctly to be generating.

These tests are to verify the generation issue on the functions tested in this code.

Signed-off-by: AraHaan <seandhunt_7@yahoo.com>
@AraHaan AraHaan force-pushed the added-missing-tests branch from efe91d6 to 89f1969 Compare May 21, 2021 02:40
@AArnott
Copy link
Member

AArnott commented May 21, 2021

Because a lot of those api's I added are technically some of the common ones I find that people want to use on the internet, myself included.

To be clear, I'm not arguing that we shouldn't generate those APIs. We do. I'm just saying they already have test coverage as part of the full-generation test. If we have a bug where certain APIs are simply missing in the generated code, then that's certainly worth taking a look at. It sounds like that's what you're saying, so I'll take another look.

this.generator = new Generator(this.metadataStream, DefaultTestGeneratorOptions, this.compilation, this.parseOptions);
Assert.True(this.generator.TryGenerate("MINIDUMP_TYPE", CancellationToken.None));
this.CollectGeneratedCode(this.generator);
Assert.True(this.generator.TryGetEnumName("MINIDUMP_TYPE", out string? actualDeclaringEnum));
Copy link
Member

Choose a reason for hiding this comment

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

You're using the wrong API here. TryGetEnumName can't be used to verify that the enum is generated.
By adding this.AssertNoDiagnostics(); to the test the generated code is logged as test output and I see that the enum is in fact generated. So no bug here, it seems.

/// <summary>
/// Verifies that MiniDumpWriteDump is generated.
/// </summary>
// TODO: Make test try to generate using x86, x64, arm, and arm64 instead of AnyCPU so that it passes.
Copy link
Member

Choose a reason for hiding this comment

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

The trick here is to add this line to the top of your test:

        this.compilation = this.compilation.WithOptions(this.compilation.Options.WithPlatform(Platform.X86));

With that, the test passes. So again, apparently no bug here.

Comment on lines +204 to +206
"GetCurrentProcess", // A handle for the current process
"GetCurrentProcessId", // An ID of the current process
"GetCurrentThreadId")] // An ID of the current thread of the current process
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the value of testing these individually unless they provide a faster path to exposing an existing test failure in the FullGeneration test.

@AArnott AArnott closed this May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants