agoda-com / agodaanalyzers Goto Github PK

Now fixer fixes the code by replacing:

return null;

with

return new List<string>();

We can improve it to support following test scenarios: (see ignored test case)
da925f8#diff-86497902145ff2fe49846efc4c54dfe5

• LinkedList
• Array
• IEnumerable

When designing our own interfaces/classes, we should not expose both synchronous and asynchronous versions of methods.

For example, if methods named MyMethod and MyMethodAsync are declared, then fail.

Fail

class Downloader
{
    public byte[] DownloadFile() { ... } // <-- disallow this declaration, because async version exists
    public Task<byte[]> DownloadFileAsync() { ... }
}

Pass

class Downloader
{
    public Task<byte[]> DownloadFileAsync() { ... }
}

Code fix

• Delete the synchronous version

https://github.agodadev.io/pages/standards-c-sharp/code-standards/async/expose-async-method.html

Could we remove the "Test only public surface for your API" rule?

It is not a good rule.

There is nothing wrong with mocking internal methods or using internal classes and unit test them. You should always try to break down complex behavior and test each part separately. If the parts work fine then you know the whole will probably work.

Allowing the testing of only public endpoints is limiting and complicates the test setup.

This rule also encourages using public which is not good.

https://github.agodadev.io/pages/standards-c-sharp/code-standards/services/service-design.html

If public class/interface method/property return value implements IEnumerable, then it can only be declared as one of the following:

• IEnumerable<T>
• ISet<T>
• IList<T>
• IDictionary<K, V>
• IReadOnlyDictionary<K, V>
• KeyedCollection<K, V>
• byte[] (special case for raw binary data)
• string (which happens to implement IEnumerable<char>)

Fail

public List<string> GetStrings() { ... }

Pass

public IList<string> GetStrings() { ... }

• https://github.agodadev.io/pages/standards-c-sharp/code-standards/collections/choosing-collection-implementation.html
• https://github.agodadev.io/pages/standards-c-sharp/code-standards/collections/arrays.html

https://github.agodadev.io/pages/standards-c-sharp/code-standards/unit-testing/only-test-the-public-interface.html

#60 is giving false positives, eg: https://github.agodadev.io/agoda-front-end/agoda-com-dictator/pull/15738

https://github.agodadev.io/agoda-front-end/agoda-com-dictator/blob/6681676d17bd66941dca1674e095f978742465a8/Src/Agoda.Website/Agoda.Website.MVC/Service/Experiment/ExperimentService.cs#L102

Seems that any string is being analyzed as a Selenium selector.

If we use this rule on Ctrl then all methods need to be appended with "Async" doesn't make sense

To do:

• only block methods on Task or ValueTask
• documentation
• Unit tests

HTTP controllers should not access query string parameters directly from HttpRequest.QueryString, they should instead leverage ASP.NET model binding

Prevent use of OpenQA.Selenium.By....() except for By.CssSelector()

https://github.agodadev.io/pages/standards-c-sharp/code-standards/gui-testing/css-selectors.html

• System.Activator.CreateInstance - any overload that takes a string
• System.Type.GetType
• any others?

https://github.agodadev.io/pages/standards-c-sharp/code-standards/code-style/reflection.html

• System.IO?
• System.Web?
• System.Net?

https://github.agodadev.io/pages/standards-c-sharp/code-standards/static-methods/when-static.html

• Task.Result
• Task.GetAwaiter

Code fix

• Rewrite as await task
• add async modifier to method declaration if it does not exist

From:

• https://github.agodadev.io/pages/standards-c-sharp/code-standards/async/await-task-result.html
• https://github.agodadev.io/pages/standards-c-sharp/code-standards/async/never-task-wait.html

Fix unescaped entities and badly formatted code examples.

Seems like rule is picking up all methods marked with attributes that derives from NUnitAttribute, resulting in false positive reporting for SetUp methods etc

I would like to prevent people from misusing the parameters as a way to return some value from a method.
For example, for following code:

public static void Method(A a){
	a.B = new B();
	a.B.C = new C();
}

both assignments are mutating the input parameter and treating it like a return value. The rule would not be applied for ref and out type params, as they imply explicitly that the parameter is / might be mutated.

Rule already exists in SonarQube, needs to be activated.

https://sonar.agodadev.io/coding_rules#qprofile=cs-agoda-frontend-c-00066|activation=false|q=HttpContext

When a webapi controller returns void, aspnet make it return a 204 instead of 200. iOS/Safari treat this differently and prompt the user for a document download blocking off the website the instant the API method is called from ajax.

We should create a rule that forces all webapi controller methods to return data and not a void.

Non-compliant code

[TestCase(1,2,3,4,5,6)]
// 6 params - must be testing too much
public void My_Test(int a, int b, int c, int d, int e, int f)
{ ... }

Compliant code

[TestCase(1,2,3,4,5)]
// maximum 5 test method params
public void My_Test(int a, int b, int c, int d, int e)
{ ... }

https://github.agodadev.io/pages/standards-c-sharp/code-standards/unit-testing/use-test-cases-appropriately.html

• Task.WaitAll
• Task.WaitAny

Code fix

• rewrite as await Task.WhenAll or await Task.WhenAny
• add async modifier to method declaration if it does not exist

https://github.agodadev.io/pages/standards-c-sharp/code-standards/code-style/dynamics.html

By the time we will get more async code that will be awesome if we have analyzers that prevent async misuse.

Nice example that we can use:
https://github.com/dotnet/roslyn-analyzers/blob/master/src/Unfactored/AsyncPackage/AsyncPackage/BlockingAsyncAnalyzer.cs

Can copy this one?

https://sonar.agodadev.io/coding_rules#qprofile=cs-agoda-frontend-c-00066|activation=false|q=HttpContext

Fail

public async Task<byte[]> DownloadFile() { ... }

Pass

public async Task<byte[]> DownloadFileAsync() { ... }

Code Fix

Add Async suffix to method name.

Classes containing test methods should not inherit from a base class.

https://github.agodadev.io/pages/standards-c-sharp/code-standards/unit-testing/be-wary-of-refactoring-tests.html

Code fix

• rewrite as await Task.Delay
• add async modifier to method declaration if it does not exist

When calling external code, if we use method TheirMethod(), but there also exists a method named TheirMethodAsync(), then fail.

Say we have an external library that exposes both sync and async versions of the same method:

// external lib
namespace AwesomeSoft
{
    public class Downloader
    {
        public byte[] DownloadFile() { ... }
        public Task<byte[]> DownloadFileAsync() { ... }
    }
}

Fail

// our code
var client = new Downloader();
var bytes = client.DownloadFile(...); // should not use synchronous version when async version available

Pass

// our code
var client = new Downloader();
var bytes = await client.DownloadFileAsync(...)

Code fix:

• Transform method name by appending Async
• Add await
• Add async modifier to method declaration if it does not exist

https://github.agodadev.io/pages/standards-c-sharp/code-standards/async/consume-async-method.html

Look at this example.

public static List<SomeThing> GetList()
{
    var object = isSomeCondition ? GetObject() : null;

    var someThings = new List<SomeThing>();
    somThings.Add(object);
    
    return someThings;
}

GetObject only return Object of SomeThing but this rules also report line var object = isSomeCondition ? GetObject() : null; as it violate the rule.

Disallow use of any method on System.Threading.Task whose name begins with Continue, eg. Task.ContinueWith, Task.ContinueWhenAll, etc.

Code fix

• Rewrite using await

https://github.agodadev.io/pages/standards-c-sharp/code-standards/async/never-task-continue-with.html

Right now AG0005 rule reports error on incorrect line - it will point out to [Test] attribute instead of actual test method signature.

Example: DependencyResolver.Current.GetService

Either XXX_YYY or XXX_YYY_ZZZ.

https://github.agodadev.io/pages/standards-c-sharp/code-standards/unit-testing/test-method-names-should-clearly-indicate-what-they-are-testing.html

https://github.agodadev.io/pages/standards-c-sharp/code-standards/collections/null-empty-enumerables.html

• Environment.MachineName
• HttpContext.Current.Server.MachineName

Gives 404 page

We want to move away from convention-based to attribute-based routing.

This analyzer should ensure that all public methods in a class that inherit from System.Web.Mvc.Controller or System.Web.Http.ApiController have a [Route] attribute.

https://agoda-com.github.io/standards-c-sharp/routing/attribute.html

This may be NUnit's Assert or one of Shouldly's. http://docs.shouldly-lib.net/v2.4.0/docs

To enable automatic notification of failing tests, and to collects stats on each team's test performance, we need to ensure that all Selenium tests are assigned to the team who is responsible for its maintenance.

When writing test case for CodeFixer, CodeFixVerifier.cs helps check if fixer introduce any new diagnostics:

https://github.com/agoda-com/AgodaAnalyzers/blob/master/src/Agoda.Analyzers.Test/Helpers/CodeFixVerifier.cs#L383

The error report, showing only new diagnostics, is quite misleading.

Example:

Before a fix:

using System;
using System.Collections.Generic;

namespace Agoda.Analyzers.Test
{
    public class TestClass
    {
        public int[] GetValuesForId(int id)
        {
            return null;
        }
    }
}

After a fix:

using System;
using System.Collections.Generic;

namespace Agoda.Analyzers.Test
{
    public class TestClass
    {
        public int[] GetValuesForId(int id)
        {
            return Array.Empty<int>();
        }
    }
}

Error report:

Message:   Fix introduced new compiler diagnostics:
Test0.cs(7,18): warning CS1591: Missing XML comment for publicly visible type or member 'TestClass'
Test0.cs(9,22): warning CS1591: Missing XML comment for publicly visible type or member 'TestClass.GetValuesForId(int)'

Actual diagnostics report:

Before a fix: using System; is unused.
After a fix: using System; is used, and this cause a different number of diagnostics returned.
We should display both before a fix and after a fix diagnostics.

Ensure argument to By.CssSelector matches the regex ^\[data-selenium=.

Fail

By.CssSelector("h1")

Pass

By.CssSelector("[data-selenium=main-heading]")

https://github.agodadev.io/pages/standards-c-sharp/code-standards/gui-testing/data-selenium.html

Code fix

• Transform to Task.Run(...);

https://github.agodadev.io/pages/standards-c-sharp/code-standards/async/task-run.html

Replace the existing implementation for AG0002 that just checks that assembly:InternalsVisibleToAttribute is not used.

https://github.agodadev.io/pages/standards-c-sharp/code-standards/unit-testing/only-test-the-public-interface.html

https://github.agodadev.io/pages/standards-c-sharp/code-standards/solution-structure/web-solution-structure.html