Skip to content
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.

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

Cleanup unit tests #212

Merged
merged 12 commits into from
Aug 18, 2019
Merged

Cleanup unit tests #212

merged 12 commits into from
Aug 18, 2019

Conversation

pBouillon
Copy link
Contributor

Goal

How

I refactored most of the tests to normalize their redaction. Using the standard AAA testing and two tools to simplify them as much as possible.

In practice

Fluent Assertions

I used FluentAssertions, most of the tests are now way more readables and may surely allow newcomers to understand better and faster how to understand the underlying logic.

For example:

// From this test case:
Assert.True(token.Length > 0 && token.Split('.').Length == 3);

// To this one:
tokenBuilt.Should()
     .NotBeEmpty("because the token should contains some data");

tokenBuilt.Split('.').Should()
     .HaveCount(3, "because the built token should have the three standard parts");

AutoFixture

https://github.com/AutoFixture/AutoFixture is a tool that allow us to specify a data type that we are going to use, without having to handle its initialization and writting magic data. It also bring more randomness in the unit tests and allow us to have a behaviour less predictable.

With this package, we can be focused only on the data we are working with and that matter for the test case.

For example:

// From this test case:
    .WithSecret("fjhsdghflghlk")

// To this one:
var secret = _fixture.Create<string>();
// ...
    .WithSecret(secret)

@abatishchev
Copy link
Member

abatishchev commented Aug 18, 2019

Thank you so much! I'm always using AAA and FA in tests myself, and often use AF too. Tests in this project needed more love for a long time.

The only change I'm not sure about is the downgrade from net462 to net461.

@abatishchev
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pBouillon
Copy link
Contributor Author

What is this failure @abatishchev ? 🤔

@abatishchev
Copy link
Member

I think the latest is net472, not 473. Can you please try that one? It should succeed then and I'll merge right away

@pBouillon
Copy link
Contributor Author

Done !

@abatishchev abatishchev merged commit a26b409 into jwt-dotnet:master Aug 18, 2019
@abatishchev
Copy link
Member

Yay, thank you again!

@pBouillon
Copy link
Contributor Author

You're welcome ! Hope I can help in a near future !

@abatishchev
Copy link
Member

If you'd like to contribute more, please help to convert the project with the .NET Framework version of tests to PackageReference/SDK-based, one of the item @glennawatson has planned to do in #211.

Another option is to get rid off then altogether as I'm not quite sure they bring much value.

What both of you think about this?

@pBouillon
Copy link
Contributor Author

I'm not very familiar with this but I would be glad to help !
Could you open an issue with more details so I can search a little how to do it ?

@glennawatson
Copy link
Contributor

The work has already been done just needs to separated into a separate pr

@glennawatson
Copy link
Contributor

@pBouillon
Copy link
Contributor Author

I'll give it a look whenever I can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants