Skip to content

Add convention test capability to Calamari - #791

Closed
N-lson wants to merge 9 commits into
masterfrom
nelson/conventions
Closed

Add convention test capability to Calamari#791
N-lson wants to merge 9 commits into
masterfrom
nelson/conventions

Conversation

@N-lson

@N-lson N-lson commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

As part of the package retention changes #team-fire-and-motion are making, we want to add convention test(s) to Calamari. For this we're taking a similar approach to OctopusServer, which uses Best.Conventional. However the current Calamari.Tests project isn't compatible as it's running net452. This PR updates Calamari.Tests to net461 along with moving the current Fixtures/Conventions tests into Fixtures/Deployment/Conventions. This is done because:

  1. This better matches the directory structure in the Calamari project which also has Fixtures/Deployment/Conventions
  2. We can put our actual convention tests in Fixtures/Conventions which aligns with OctopusServer's structure

How to review

The main change in this PR is in Calamari.Tests.csproj. All .cs files should just be namespace changes with the exception of test fixtures which have been excluded from running on Mono <5.12.0 due to HttpClient issues (I've commented these on the PR to make them easier to find amongst the mass of namespace changes).

This PR also adds a global.json file which is required to build the solution on our TeamCity agents using .NET SDK 5 instead of .NET SDK 3.1 which leads to incompatibility issues with net461.

To go along with this PR we have a docs change to update our earliest recommended version of Mono OctopusDeploy/docs#1390

@N-lson
N-lson requested a review from a team November 15, 2021 02:48
[Test]
[RequiresMonoVersion480OrAboveForTls12]
[RequiresMinimumMonoVersion(5, 12, 0, Description = "HttpClient 4.3.2 broken on Mono - https://xamarin.github.io/bugzilla-archives/60/60315/bug.html#c7")]
public void ShouldDownloadPackageWithRepositoryMetadata()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test now requires Mono >= 5.12.0

[NonParallelizable]
[RequiresNonFreeBSDPlatform(SkipFreeBsdBecause)]
[RequiresMinimumMonoVersion(5, 12, 0, Description = "HttpClient 4.3.2 broken on Mono - https://xamarin.github.io/bugzilla-archives/60/60315/bug.html#c7")]
public void DoesNotTimeOutIfTheServerRespondsBeforeTheTimeout()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test now requires Mono >= 5.12.0

[NonParallelizable]
[RequiresNonFreeBSDPlatform(SkipFreeBsdBecause)]
[RequiresMinimumMonoVersion(5, 12, 0, Description = "HttpClient 4.3.2 broken on Mono - https://xamarin.github.io/bugzilla-archives/60/60315/bug.html#c7")]
public void IgnoresTheTimeoutIfAnInvalidTimeoutIsDefinedInVariables()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test now requires Mono >= 5.12.0

[NonParallelizable]
[RequiresNonFreeBSDPlatform(SkipFreeBsdBecause)]
[RequiresMinimumMonoVersion(5, 12, 0, Description = "HttpClient 4.3.2 broken on Mono - https://xamarin.github.io/bugzilla-archives/60/60315/bug.html#c7")]
public void TimesOutIfAValidTimeoutIsDefinedInVariables()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test now requires Mono >= 5.12.0

@N-lson
N-lson requested a review from andrewabest November 17, 2021 04:17
Comment thread global.json
@@ -0,0 +1,6 @@
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this for this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm we did need this when jumping to net480 since the 3.1 SDK wasn't compatible with building the tests. But for net461 I'm not certain. I can check when I split the PR up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update: The tests built fine without this change on net461, so looks like no it's not needed.

using NUnit.Framework;

namespace Calamari.Tests.Fixtures.Conventions
namespace Calamari.Tests.Fixtures.Deployment.Conventions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This namespace shuffle has added quite a lot of churn / noise to the PR. Perhaps we could separate this into two PRs: one that introduces the framework changes, and another that then does the "tidy up" of namespace reshuffling? It will make them easier to understand later on if people need to return to them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me

@andrewabest andrewabest left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've made a suggestion to reorganise this work into two PRs so that they are a bit smaller / more focussed. When that is done I think they will both be good to go :)

@N-lson

N-lson commented Nov 17, 2021

Copy link
Copy Markdown
Contributor Author

The changes have been split into the above PRs ☝️

@N-lson N-lson closed this Nov 17, 2021
@N-lson
N-lson deleted the nelson/conventions branch November 17, 2021 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants