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
wants to merge 4 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 all commits
Commits
Show all changes
4 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
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

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

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

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

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.

#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>
#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>
#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