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

test: Migrate all the template tests to Junit5 #3986

Merged
merged 1 commit into from
Jun 14, 2021
Merged

test: Migrate all the template tests to Junit5 #3986

merged 1 commit into from
Jun 14, 2021

Conversation

Rohitesh-Kumar-Jain
Copy link
Contributor

#3919

Hi, this PR involves pure refactoring.

I will be working on templateTest next hence raised the PR for migration.

In one of my previous migrations, @slarse gave a green flag for migration PR(s) involving 100 changes (50 additions - 50 deletions), so as I needed to migrate templateTest, I thought it would be nice to migrate all the tests in the template directory.

Ik this PR involves 61 additions & deletions which are more than suggested, but the changes are pretty simple.

This PR just involves these three types of changes:

  1. changing imports

  2. In some assertions due to the Junit5 rules, the order of the parameters changed with the output message parameter moved as the last parameter.

  3. Junit5 doesn't support assertThat, so I have replaced its import with import static org.hamcrest.MatcherAssert.assertThat; this is also suggested in the official Junit5 documentation.

I hope 61 additions will not be difficult to review 😅

@Rohitesh-Kumar-Jain
Copy link
Contributor Author

Few files in this PR only involved 2-3 changes so I feel it's better to club them.

Ik I am supposed to improve the test strength of the tests, next I will raise a PR for that #3967

@slarse
Copy link
Collaborator

slarse commented Jun 11, 2021

In one of my previous migrations, @slarse gave a green flag for migration PR(s) involving 100 changes (50 additions - 50 deletions)

That was a number I grabbed out of the air just to give rough order of magnitude. 100 changes or 120 changes: same thing different name :)

so as I needed to migrate templateTest, I thought it would be nice to migrate all the tests in the template directory.

Good call!

Junit5 doesn't support assertThat, so I have replaced its import with import static org.hamcrest.MatcherAssert.assertThat;

Entirely correct 👍

I gave it a glance and it looks good, but I'll wait for CI to pass and probably review this tomorrow.

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

This all looks good to me. @Rohitesh-Kumar-Jain well made decisions here, this is an easy to review PR :)

@monperrus ping for merge

@monperrus monperrus changed the title review: test: Migrate all the template tests to Junit5 test: Migrate all the template tests to Junit5 Jun 14, 2021
@monperrus monperrus merged commit 2b95b7a into INRIA:master Jun 14, 2021
@monperrus
Copy link
Collaborator

Very nice, thanks @Rohitesh-Kumar-Jain

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.

None yet

3 participants