Skip to content

Engineering guidelines

Jump to bottom
Tanay Parikh edited this page Apr 9, 2022 · 41 revisions

Engineering guidelines

Basics

Copyright header and license notice

All source code files (mostly src/**/*.cs and test/**/*.cs) require this exact header (please do not make any changes to it):

// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

It is ok to skip it on generated files, such as *.designer.cs.

External dependencies

This refers to dependencies on projects (i.e. NuGet packages) outside of the aspnetcore repo, and especially outside of Microsoft. Because these repos are a core part of all ASP.NET Core/EF apps, it is important that we be careful and manage our dependencies properly.

3rd party software needs to be approved for us and registered at https://tools.opensource.microsoft.com/use. Ensure that the THIRD-PARTY-NOTICES.txt file in the repository root is updated to include the software license.

Code reviews and checkins

To help ensure that only the highest quality code makes its way into the project, please submit all your code changes to GitHub as PRs. This includes runtime code changes, and test updates.

The advantages are numerous: improving code quality, more visibility on changes and their potential impact, avoiding duplication of effort, and creating general awareness of progress being made in various areas.

In general a PR should be signed off (using GitHub's "approve" feature) by the Subject Matter Expert (SME) of that code. For example, a change to the Banana project should be signed off by @MrMonkey, and not by @MrsGiraffe. If you don't know the SME, please talk to one of the engineering leads and they will be happy to help you identify the SME. Of course, sometimes it's the SME who is making a change, in which case a secondary person will have to sign off on the change (e.g. @JuniorMonkey).

To commit the PR to the repo use GitHub's "Squash and Merge" button on the main PR page.

Build numbers

By default, builds of ASP.NET Core projects on a local developer machine will produce the same version every time you execute the build.ps1 or build.sh scripts. e.g. 2.1.0-preview1-t000.

To change the build number, you can either:

  1. Pass in the build number explicitly 
    ./build.ps1 /p:BuildNumber=1234
  2. Set an environment variable 
    $env:BUILD_NUMBER='1234'
./build.ps1
  3. Set "IncrementalVersion" to produce a time-based build number each time the build is produced. 
    ./build.ps1 /p:IncrementalVersion=true

Branch strategy

In general:

  • master has the code that is being worked on but not yet released. This is the branch into which developers normally submit pull requests and merge changes into.
    • Currently master contains work for the 3.0 release.
  • release/<major>.<minor> contains code that is intended for release. The <major>.<minor> part of the branch name indicates the beginning of the version of the product the code will end up in. The branch may be used for several patch releases with the same major.minor number.
    • External contributors do not normally need to make pull requests to these branches. However, when multiple releases are in simultaneous development, targeting a release branch may be preferred. Check with a feature area owner before sending a PR if you are unsure about the branch to target.

Solution and project folder structure and naming

AspNetCore uses a unified solution file and solution filters for feature areas. When adding a new project to the repo, ensure that it's added to the solution and to the solution filter. You may also need to update solution filter files (slnf) for other areas that may reference this new project.

Conditional compilation for multiple Target Frameworks

Code sometimes has to be written to be target framework-specific due to API changes between frameworks. Use #if statements to create these conditional code segments: Desktop:

#if NETCOREAPP5_0
            Console.WriteLine("Hi .NET 8.0");
#elif NETCOREAPP
            Console.WriteLine("Hi older NETCOREAPP TFMs");
#else
#error Target framework needs to be updated
#endif

Note the #error section that is present in case the target frameworks change - this ensure that we don't have dead code in the projects, and also no missing conditions.

Assembly naming pattern

The general naming pattern is Microsoft.AspNetCore.<area>.<subarea>, Microsoft.EntityFrameworkCore.<area>.<subarea>, and Microsoft.Extensions.<area>.<subarea>.

Unit tests

We use xUnit.net for testing most code. Jasmine and Jest are used for unit testing TypeScript or JavaScript code.

Coding guidelines

❕ The content of the code that we write.

Coding style guidelines – general

The most general guideline is that we use all the VS default settings in terms of code formatting, except that we put System namespaces before other namespaces (this used to be the default in VS, but it changed in a more recent version of VS).

  1. Use four spaces of indentation (no tabs)
  2. Use _camelCase for private fields
  3. Avoid this. unless absolutely necessary
  4. Always specify member visibility, even if it's the default (i.e. private string _foo; not string _foo;)
  5. Open-braces ({) go on a new line
  6. Use any language features available to you (expression-bodied members, throw expressions, tuples, etc.) as long as they make for readable, manageable code.
    • This is pretty bad: public (int, string) GetData(string filter) => (Data.Status, Data.GetWithFilter(filter ?? throw new ArgumentNullException(nameof(filter))));

Usage of the var keyword

The var keyword is to be used as much as the compiler will allow. For example, these are correct:

var fruit = "Lychee";
var fruits = new List<Fruit>();
var flavor = fruit.GetFlavor();
string fruit = null; // can't use "var" because the type isn't known (though you could do (string)null, don't!)
const string expectedName = "name"; // can't use "var" with const

The following are incorrect:

string fruit = "Lychee";
List<Fruit> fruits = new List<Fruit>();
FruitFlavor flavor = fruit.GetFlavor();

Use C# type keywords in favor of .NET type names

When using a type that has a C# keyword the keyword is used in favor of the .NET type name. For example, these are correct:

public string TrimString(string s)
{
    return string.IsNullOrEmpty(s)
        ? null
        : s.Trim();
}

var intTypeName = nameof(Int32); // can't use C# type keywords with nameof

The following are incorrect:

public String TrimString(String s) 
{
    return String.IsNullOrEmpty(s)
        ? null
        : s.Trim();
}

Cross-platform coding

Our frameworks should work on CoreCLR, which supports multiple operating systems. Don't assume we only run (and develop) on Windows. Code should be sensitive to the differences between OS's. Here are some specifics to consider.

Line breaks

Windows uses \r\n, OS X and Linux uses \n. When it is important, use Environment.NewLine instead of hard-coding the line break.

Note: this may not always be possible or necessary.

Be aware that these line-endings may cause problems in code when using @"" text blocks with line breaks.

Environment Variables

OS's use different variable names to represent similar settings. Code should consider these differences.

For example, when looking for the user's home directory, on Windows the variable is USERPROFILE but on most Linux systems it is HOME.

var homeDir = Environment.GetEnvironmentVariable("USERPROFILE") 
                  ?? Environment.GetEnvironmentVariable("HOME");

File path separators

Windows uses \ and OS X and Linux use / to separate directories. Instead of hard-coding either type of slash, use Path.Combine() or Path.DirectorySeparatorChar.

If this is not possible (such as in scripting), use a forward slash. Windows is more forgiving than Linux in this regard.

When to use internals vs. public and when to use InternalsVisibleTo

Use InternalsVisibleTo when sharing code between types in the same assembly, same feature area, or to unit test internal types and members. If two runtime assemblies need to share common helpers then we will use a "shared source" solution with build-time only packages. Check out the some of the projects in https://github.com/aspnet/Common/ and how they are referenced from other solutions.

If two runtime assemblies need to call each other's APIs, consider making the APIs public or if there's enough extensibility for a customer to perform something similar. If we need it, it is likely that our customers need it.

Async method patterns

By default all async methods must have the Async suffix. There are some exceptional circumstances where a method name from a previous framework will be grandfathered in.

Passing cancellation tokens is done with an optional parameter with a value of default(CancellationToken), which is equivalent to CancellationToken.None (one of the few places that we use optional parameters). The main exception to this is in web scenarios where there is already an HttpContext being passed around, in which case the context has its own cancellation token that can be used when needed.

Sample async method:

public Task GetDataAsync(
    QueryParams query,
    int maxData,
    CancellationToken cancellationToken = default(CancellationToken))
{
    ...
}

Extension method patterns

The general rule is: if a regular static method would suffice, avoid extension methods.

Extension methods are often useful to create chainable method calls, for example, when constructing complex objects, or creating queries.

Internal extension methods are allowed, but bear in mind the previous guideline: ask yourself if an extension method is truly the most appropriate pattern.

The namespace of the extension method class should generally be the namespace that represents the functionality of the extension method, as opposed to the namespace of the target type. One common exception to this is that the namespace for middleware extension methods are normally always the same as the namespace of IAppBuilder.

The class name of an extension method container (also known as a "sponsor type") should generally follow the pattern of <Feature>Extensions, <Target><Feature>Extensions, or <Feature><Target>Extensions. For example:

namespace Food 
{
    class Fruit { ... }
}

namespace Fruit.Eating 
{
    class FruitExtensions 
    { 
        public static void Eat(this Fruit fruit); 
    }
  OR
    class FruitEatingExtensions 
    { 
        public static void Eat(this Fruit fruit); 
    }
  OR
    class EatingFruitExtensions 
    { 
        public static void Eat(this Fruit fruit); 
    }
}

When writing extension methods for an interface the sponsor type name must not start with an I.

Doc comments

The person writing the code will write the doc comments. Public APIs only. No need for doc comments on non-public types.

Note: Public means callable by a customer, so it includes protected APIs. However, some public APIs might still be "for internal use only" but need to be public for technical reasons. We will still have doc comments for these APIs but they will be documented as appropriate.

Unit tests and functional tests

Assembly naming

The unit tests for the Microsoft.Fruit assembly live in the Microsoft.Fruit.Tests assembly.

The functional tests for the Microsoft.Fruit assembly live in the Microsoft.Fruit.FunctionalTests assembly.

In general there should be exactly one unit test assembly for each product runtime assembly. In general there should be one functional test assembly per repo. Exceptions can be made for both.

Unit test class naming

Test class names end with Test and live in the same namespace as the class being tested. For example, the unit tests for the Microsoft.Fruit.Banana class would be in a Microsoft.Fruit.BananaTest class in the test assembly.

Unit test method naming

Unit test method names must be descriptive about what is being tested, under what conditions, and what the expectations are. Pascal casing and underscores can be used to improve readability. The following test names are correct:

PublicApiArgumentsShouldHaveNotNullAnnotation
Public_api_arguments_should_have_not_null_annotation

The following test names are incorrect:

Test1
Constructor
FormatString
GetData

Unit test structure

The contents of every unit test should be split into three distinct stages, optionally separated by these comments:

// Arrange  
// Act  
// Assert

The crucial thing here is that the Act stage is exactly one statement. That one statement is nothing more than a call to the one method that you are trying to test. Keeping that one statement as simple as possible is also very important. For example, this is not ideal:

int result = myObj.CallSomeMethod(GetComplexParam1(), GetComplexParam2(), GetComplexParam3());

This style is not recommended because way too many things can go wrong in this one statement. All the GetComplexParamN() calls can throw for a variety of reasons unrelated to the test itself. It is thus unclear to someone running into a problem why the failure occurred.

The ideal pattern is to move the complex parameter building into the Arrange section:

// Arrange
P1 p1 = GetComplexParam1();
P2 p2 = GetComplexParam2();
P3 p3 = GetComplexParam3();

// Act
int result = myObj.CallSomeMethod(p1, p2, p3);

// Assert
Assert.AreEqual(1234, result);

Now the only reason the line with CallSomeMethod() can fail is if the method itself blew up. This is especially important when you're using helpers such as ExceptionHelper, where the delegate you pass into it must fail for exactly one reason.

Testing exception messages

In general testing the specific exception message in a unit test is important. This ensures that the exact desired exception is what is being tested rather than a different exception of the same type. In order to verify the exact exception it is important to verify the message.

To make writing unit tests easier it is recommended to compare the error message to the RESX resource. However, comparing against a string literal is also permitted.

var ex = Assert.Throws<InvalidOperationException>(
    () => fruitBasket.GetBananaById(1234));
Assert.Equal(
    Strings.FormatInvalidBananaID(1234),
    ex.Message);

Use xUnit.net's plethora of built-in assertions

xUnit.net includes many kinds of assertions – please use the most appropriate one for your test. This will make the tests a lot more readable and also allow the test runner report the best possible errors (whether it's local or the CI machine). For example, these are bad:

Assert.Equal(true, someBool);

Assert.True("abc123" == someString);

Assert.True(list1.Length == list2.Length);

for (int i = 0; i < list1.Length; i++) 
{
    Assert.True(
        String.Equals
            list1[i],
            list2[i],
            StringComparison.OrdinalIgnoreCase));
}

These are good:

Assert.True(someBool);

Assert.Equal("abc123", someString);

// built-in collection assertions!
Assert.Equal(list1, list2, StringComparer.OrdinalIgnoreCase);

Parallel tests

By default all unit test assemblies should run in parallel mode, which is the default. Unit tests shouldn't depend on any shared state, and so should generally be runnable in parallel. If the tests fail in parallel, the first thing to do is to figure out why; do not just disable parallel tests!

For functional tests it is reasonable to disable parallel tests.

Use only complete words or common/standard abbreviations in public APIs

Public namespaces, type names, member names, and parameter names must use complete words or common/standard abbreviations.

These are correct:

public void AddReference(AssemblyReference reference);
public EcmaScriptObject SomeObject { get; }

These are incorrect:

public void AddRef(AssemblyReference ref);
public EcmaScriptObject SomeObj { get; }

Common Patterns

This section contains common patterns used in our code.

Logging patterns

  1. Always specify an EventId. Include a numeric ID and a name. The name should be a PascalCasedCompoundWord (i.e. no spaces, and each "word" within the name starts with a capital letter).

  2. In production code, use "pre-compiled logging functions" (see below). Test code can use any kind of logging

  3. Prefer defining pre-compiled messages in a static class named Log that is a nested class within the class you are logging from. Messages that are used by multiple components can be defined in a shared class (but this is discouraged).

  4. Consider separating the Log nested class into a separate file by using partial classes. Name the additional file [OriginalClassName].Log.cs

  5. Never use string-interpolation ($"Foo {bar}") for log messages. Log message templates are designed so that structured logging systems can make individual parameters queryable and string-interpolation breaks this.

  6. Always use PascalCasedCompoundWords as template replacement tokens.

Pre-compiled Logging Functions

Production code should use "pre-compiled" logging functions. This means using LoggerMessage.Define to create a compiled function that can be used to perform logging. For example, consider the following log statement:

public class MyComponent
{
	public void MyMethod()
	{
		_logger.LogError(someException, new EventId(1, "ABadProblem"), "You are having a bad problem and will not go to {Location} today", "Space");
	}
}

The logging infrastructure has to parse the template ("You are having a bad problem and will not go to {Location} today") every time the log is written. A pre-compiled logging function allows you to compile the template once and get back a delegate that can be invoked to log the message without requiring the template be re-parsed. For example:

public class MyComponent
{
	public void MyMethod()
	{
		Log.ABadProblem(_logger, "Space", someException);
	}

	private static class Log
	{
		private static readonly Action<ILogger, string, Exception> _aBadProblem =
			LoggerMessage.Define<string>(
				LogLevel.Error,
				new EventId(2, "ABadProblem"), 
				"You are having a bad problem and will not go to {Location} today");

		public static void ABadProblem(ILogger logger, string location, Exception ex) => _aBadProblem(logger, location, ex);
	}
}

If MyComponent is a large class, consider splitting the Log nested class into a separate file:

MyComponent.cs

public partial class MyComponent
{
	public void MyMethod()
	{
		Log.ABadProblem(_logger, "Space", someException);
	}
}

MyComponent.Log.cs

public partial class MyComponent
{
	private static class Log
	{
		private static readonly Action<ILogger, string, Exception> _aBadProblem =
			LoggerMessage.Define<string>(
				LogLevel.Error,
				new EventId(2, "ABadProblem"), 
				"You are having a bad problem and will not go to {Location} today");

		public static void ABadProblem(ILogger logger, string location, Exception ex) => _aBadProblem(logger, location, ex);
	}
}

Testing PRs

Though it's expected that you run tests locally for your PR, tests will also run on any PR you send to GitHub. Test are required to pass as part of build checks before a PR can be merged.

Product planning and issue tracking

❕ How we track what work there is to do.

Issue tracking

Bug management takes place in GitHub. We use area labels to tag issues to their feature areas. https://github.com/dotnet/aspnetcore/blob/master/docs/TriageProcess.md goes in more detail about our triage and issue management process.

Breaking changes

In general, breaking changes can be made only in a new major product version, e.g. moving from 1.x.x to 2.0.0. Even still, we generally try to avoid breaking changes because they can incur large costs for anyone using these products. All breaking changes must be approved as part of the API review process.

For the normal case of breaking changes in major versions, this is the ideal process:

  1. Provide some new alternative API (if necessary)
  2. Mark the old type/member as [Obsolete] to alert users (see below), and to point them at the new alternative API (if applicable)
  • If the old API really doesn't/can't work at all, please discuss with engineering team
  1. Update the XML doc comments to indicate the type/member is obsolete, plus what the alternative is. This is typically exactly the same as the obsolete attribute message.
  2. File a bug in the next major milestone (e.g. 2.0.0) to remove the type/member
  • Mark this bug with a red [breaking-change] label (use exact casing, hyphenation, etc.). Create the label in the repo if it's not there.

Example of obsoleted API:

    /// <summary>
    /// <para>
    ///     This method/property/type is obsolete and will be removed in a future version.
    ///     The recommended alternative is Microsoft.SomethingCore.SomeType.SomeNewMethod.
    /// </para>
    /// <para>
    ///     ... old docs...
    /// </para>
    /// </summary>
    [Obsolete("This method/property/type is obsolete and will be removed in a future version. The recommended alternative is Microsoft.SomethingCore.SomeType.SomeNewMethod.")]
    public void SomeOldMethod(...)
    {
        ...
    }

GitHub Flavored Markdown

GitHub supports Markdown in many places throughout the system (issues, comments, etc.). However, there are a few differences from regular Markdown that are described here:

https://help.github.com/articles/github-flavored-markdown

Including people in a GitHub discussion

To include another team member in a discussion on GitHub you can use an @ mention to cause a notification to be sent to that person. This will automatically send a notification email to that person (assuming they have not altered their GitHub account settings). For example, in a PR's discussion thread or in an issue tracker comment you can type @username to have them receive a notification. This is useful when you want to "include" someone in a code review in a PR, or if you want to get another opinion on an issue in the issue tracker.

Clone this wiki locally