Skip to content

Adding headers to test common header files #5903

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.

Sign up for GitHub

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

Open
guimafelipe wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Adding headers to test common header files #5903

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ee97ff
Fixing the headers for the common test header files
guimafelipe Oct 14, 2025
26bc944
Fixing this one
guimafelipe Oct 14, 2025
54f6d09
Merge branch 'main' into user/felipeda/testheaders
guimafelipe Oct 17, 2025
dd2943e
Fixing include order
guimafelipe Oct 19, 2025
50adacc
Merge branch 'main' into user/felipeda/testheaders
guimafelipe Dec 1, 2025
8a773ee
Using more specific wex headers
guimafelipe Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion test/inc/TestDef.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// Copyright (c) Microsoft Corporation and Contributors.
// Copyright (c) Microsoft Corporation and Contributors.
// Licensed under the MIT License.
#pragma once

#include <windows.h>
#include <string>
#include <winrt/base.h>

constexpr static const int c_phaseTimeout = (30 * 1000); // 30 seconds
static const std::wstring c_genericTestMoniker = L"this_is_a_test";
static const std::wstring c_testFailureEventName = L"WindowsAppRuntimeTestFailureEventName";
Expand Down
4 changes: 2 additions & 2 deletions test/inc/WindowsAppRuntime.Test.AppModel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright (c) Microsoft Corporation and Contributors.
// Copyright (c) Microsoft Corporation and Contributors.
// Licensed under the MIT License.

#ifndef __WINDOWSAPPRUNTIME_TEST_APPMODEL_H
#define __WINDOWSAPPRUNTIME_TEST_APPMODEL_H

#include <windows.h>
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 19, 2025

Choose a reason for hiding this comment

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

Why?

Is this currently a problem or is this a stylistic change?

This is the most obvious example - why does this need to include windows.h? That's routinely the 1st include in any precompiled header, and in any source file in projects not using precompiled headers. Also, some include <unknown.h> rather than windows.h (the former includes the latter, plus other things).

I don't see the value including windows.h in any header file (except precomp.h/pch.h)

Copy link
Contributor Author

@guimafelipe guimafelipe Oct 19, 2025

Choose a reason for hiding this comment

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

The current problem is that for some of these headers, you can't include them alone without including these dependencies before in your file. Creating a new test that uses one of these headers requires knowing what they depend on, and it is not possible to just include the header that you actually want to use alone, requiring the include of windows.h before including the desire header for example.
I think it is a good practice to have headers that work as their own (by being possible to compile a .cpp file including only it, which is what I mean when I say a header can be compiled) and it would make it easier for others to use them within the project.

For this specific case, this header can be compile on its own because it includes windows.h within appmodel.h, but it is hidden under a conditional macro. So I included it here to make it safer.

Do you think there are any downsides on doing this change? We can close this PR if it is not desired.

Copy link
Member

@DrusTheAxe DrusTheAxe Oct 27, 2025

Choose a reason for hiding this comment

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

There may be some merits to parts of it, but as is it's too broad a change e.g. don't add windows.h to any header file (e.g. cause complications if someone uses unknown.h). There's a hard balance here as there's also the problem where includes can intersect in...interesting ways, if not identical includes and order, and headers including headers make any removal of includes problematic as the header evolves e.g. a.h includes b.h because it need something, but later a.h is split into a.h and x.h and now b.h isn't needed in a.h...but someone who included a.h assumed b.h's were available and now you create downstream build breaks, even though your code using a.h works fine TL;DR easy to add includes , hard to remove.

I'd be more receptive to a PR making changes specifically because a new test was being stood up and it showed build errors because of some assumed includes, in which case (perhaps) add (some?) of those in these headers.

Copy link
Contributor Author

@guimafelipe guimafelipe Oct 27, 2025

Choose a reason for hiding this comment

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

a.h includes b.h because it need something, but later a.h is split into a.h and x.h and now b.h isn't needed in a.h...but someone who included a.h assumed b.h's were available and now you create downstream build breaks

Isn't this desirable? For a simple example, we can have a.h including <string>:

// a.h
#pragma once
#include <string>

And then, another header includes a.h and uses the <string> that is exposed by it:

// some-header.h
#pragma once
#include "a.h"

std::wstring GetName();

If suddenly a.h removes the #include <string>, the files that only includes some-header.h and not <string> will break. But that break will be at compile time and will be correct in my opinion because some-header.h should be including <string> as it is a direct dependency. So this build break will force a change in the correct direction, stopping a usage of a indirect include that was not clear.

Copy link
Member

@ChrisGuzak ChrisGuzak Dec 1, 2025

Choose a reason for hiding this comment

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

this is per C++ core guidelines and this too

if you can include less than windows.h please do

Copy link
Contributor Author

@guimafelipe guimafelipe Dec 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The thing is that there headers uses type definitions and functions that comes from header internal from windows.h. And these internal headers are dependent on the macros defined in windows.h to function.

For example, here on WindowsAppRuntime.Test.AppModel.h, the type UINT32 is defined in basetsd.h Which behaves well if included directly.

But on WindowsAppRuntime.Test.TAEF.h for example, we use the PCWSTR and WCHAR types, which are both defined in winnt.h. This header depends of an architecture (something like _AMD64_) being defined, or it errors with #error "No Target Architecture". And this is defined by the windows.h header. The same would happen with we just include the minwindef.h header that internally includes winnt.h.

I went to the windows.h header here because that is what essentially being included in the pch.h file for the compilation of the tests alongside with unknwn.h in some of them (which internally includes windows.h on line 29).
Some of the tests include windows.h right after a #define WIN32_LEAN_AND_MEAN to exclude rerely-used stuff, which can also help to keep things small there.

Any thoughts or suggestions?
Thank you.

Copy link
Member

@DrusTheAxe DrusTheAxe Dec 9, 2025

Choose a reason for hiding this comment

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

@ChrisGuzak do those guidelines also apply to windows.h? I'd generally find it extremely-weird to see code #include'ing basetsd.h, minwindef.h or other parts of windows.h. And there doesn't seem a lot of merit #include'ing windows.h in headers given nearly every .h uses basic constructs from windows.h (PCWSTR, UINT32, DWORD, ...). Perhaps this is worthy of discussion at our next API Design meeting?

Some of the tests include windows.h right after a #define WIN32_LEAN_AND_MEAN to exclude rerely-used stuff

@guimafelipe Any such definitions should occur in pch.h before any #include (pretty much the only thing I'd expect to see in pch.h between line1 //Copyright... and line2+ #include's)

TBH I never heard of unknown.h before working in WinAppSDK 0.x, nor would I have expected the weird build errors mixing unknown.h vs windows.h if I it didn't draw blood on multiple occasions :-( Could be a dated problem buit this isn't the sort of stuff that changes often or quickly, so I'm hesitant to slather windows.h everywhere. Not without a better understanding of the merit (like, discussion at our next API meeting).

#include <appmodel.h>

#include <Verify.h>

namespace Test::AppModel
Expand Down
1 change: 1 addition & 0 deletions test/inc/WindowsAppRuntime.Test.Bootstrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#ifndef __WINDOWSAPPRUNTIME_TEST_BOOTSTRAP_H
#define __WINDOWSAPPRUNTIME_TEST_BOOTSTRAP_H

#include <windows.h>
Copy link
Member

@DrusTheAxe DrusTheAxe Nov 20, 2025

Choose a reason for hiding this comment

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

Don't include windows.h. As mentioned that's not always the right answer, and it's such a universal pervasive thing it's not helpful here and can be harmful

#include <MddBootstrap.h>
#include <MddBootstrapTest.h>

Expand Down
4 changes: 3 additions & 1 deletion test/inc/WindowsAppRuntime.Test.FileSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// Copyright (c) Microsoft Corporation and Contributors.
// Copyright (c) Microsoft Corporation and Contributors.
// Licensed under the MIT License.

#ifndef __WINDOWSAPPRUNTIME_TEST_FILESYSTEM_H
#define __WINDOWSAPPRUNTIME_TEST_FILESYSTEM_H

#include <windows.h>
#include <filesystem>
#include <verify.h>

#include <wil/win32_helpers.h>
#include <wil/resource.h>
Expand Down
1 change: 1 addition & 0 deletions test/inc/WindowsAppRuntime.Test.Package.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#ifndef __WINDOWSAPPRUNTIME_TEST_PACKAGE_H
#define __WINDOWSAPPRUNTIME_TEST_PACKAGE_H

#include <windows.h>
#include <appmodel.h>

#include <WindowsAppRuntime.Test.FileSystem.h>
Expand Down
6 changes: 5 additions & 1 deletion test/inc/WindowsAppRuntime.Test.TAEF.cppwinrt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
// Copyright (c) Microsoft Corporation and Contributors. All rights reserved.
// Copyright (c) Microsoft Corporation and Contributors. All rights reserved.
// Licensed under the MIT License.

#ifndef __WINDOWSAPPRUNTIME_TEST_TAEF_CPPWINRT_H
#define __WINDOWSAPPRUNTIME_TEST_TAEF_CPPWINRT_H

#include <windows.h>
#include <winrt/base.h>
Copy link
Member

@DrusTheAxe DrusTheAxe Nov 20, 2025

Choose a reason for hiding this comment

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

This leaves me uneasy for reasons I can't quite articulate at the moment (perhaps not enough coffee?)

Aha. Found it. The source of my uneasiness...

Is base.h a well defined part of C++/WinRT's API or just an implementation detail? e.g. https://learn.microsoft.com/en-us/uwp/cpp-ref-for-winrt/hstring but nowhere on that page does it say the header(s) you need to include (unlike, say, CreateFileW docs say what header (+lib) is needed), nor is it mentioned in any example.

I know about this header, but I've pored over C++/WinRT's generated headers and implementation in more detail than most. Is winrt/base.h part of the official API or an implementation detail? If the former it should be mentioned in the docs, but it's not. If the latter it's probably not something we should directly reference.

Copy link
Contributor Author

@guimafelipe guimafelipe Dec 1, 2025

Choose a reason for hiding this comment

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

The first mention of it I have is here, where it shows how to include winrt/base.h in the pre-compiled header. So I assume it is public and intended to be included.

#include <WexTestClass.h>

namespace WEX::TestExecution
{
// Teach TAEF how to format a winrt::hstring
Expand Down
6 changes: 5 additions & 1 deletion test/inc/WindowsAppRuntime.Test.TAEF.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
// Copyright (c) Microsoft Corporation and Contributors. All rights reserved.
// Copyright (c) Microsoft Corporation and Contributors. All rights reserved.
// Licensed under the MIT License.

#ifndef __WINDOWSAPPRUNTIME_TEST_TAEF_H
#define __WINDOWSAPPRUNTIME_TEST_TAEF_H

#include <windows.h>
#include <filesystem>
#include <WexTestClass.h>

namespace Test::TAEF
{
inline std::filesystem::path GetDeploymentDir()
Expand Down
Loading