Engineering‐guidelines - ganweisoft/WebPlugins GitHub Wiki

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

// Copyright (c) OpenTOMs. All rights reserved. // Licensed under the MIT License, See License.txt in the project root for license information. It is ok to skip it on generated files, such as *.designer.cs.

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

3rd party software needs to be approved for us . Ensure that the THIRD-PARTY-NOTICES.txt file in the repository root is updated to include the software license.

To help ensure that only the highest quality code makes its way into the project, please submit all your code changes to Gitee 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 Gitee's "approve" feature) by the Subject Matter Expert (SME) of that code. 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

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

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/. contains code that is intended for release. The . 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.

OpenTOMs 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.

The general naming pattern is .IoTCenter.Module.

We use Microsoft.UnitTest for testing most code.

❕ The content of the code that we write.

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).

Use four spaces of indentation (no tabs) Use _camelCase for private fields Avoid this. unless absolutely necessary Always specify member visibility, even if it's the default (i.e. private string _foo; not string _foo;) Open-braces ({) go on a new line 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))));

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(); 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 fruits = new List(); FruitFlavor flavor = fruit.GetFlavor();

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(); }

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.

The unit tests for the OpenTOMs.Fruit assembly live in theOpenTOMs.Fruit.Tests assembly.

The functional tests for the OpenTOMs.Fruit assembly live in the OpenTOMs.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.

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

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

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.

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( () => fruitBasket.GetBananaById(1234)); Assert.Equal( Strings.FormatInvalidBananaID(1234), ex.Message);

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.

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; }

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

❕ How we track what work there is to do.

Bug management takes place in Gitee. We use area labels to tag issues to their feature areas. [Issues · 深圳市敢为软件技术有限公司/OpenTOMs - Gitee.com](https://gitee.com/shend/OpenTOMs/issues) goes in more detail about our triage and issue management process.

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:

Provide some new alternative API (if necessary) 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 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. 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(...)
{
    ...
}

To include another team member in a discussion on Gitee 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 Gitee 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.