Skip to content

refactor test in the common package #13

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

refactor test in the common package #13

wants to merge 7 commits into from

Conversation

@ignatmaloukhov
Copy link

Refactored tests in the common package

mipo256
mipo256 requested changes May 25, 2025
View reviewed changes
Copy link
Member

@mipo256 mipo256 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR.

The only thing that I think I would want to ask to change is to keep using assertj for assertions. From my opinion, the assertj api is more clean in terms of what is expected, and what is actual argument, and is more clear in assertion failure messages.

Also, do you mind taking a look into the other test classes as well?

@mipo256
Copy link
Member

mipo256 commented May 25, 2025

Closes #11

@mipo256 mipo256 linked an issue May 25, 2025 that may be closed by this pull request
Adopt a single naming convention in unit tests #11
Open
@ignatmaloukhov
Copy link
Author

assertions

Thanks for the review.

This was the first try to get your opinion on the proposed changes. I am ready to complete all the test classes of course.

I returned assert back for assertions.

@mipo256
Copy link
Member

mipo256 commented May 25, 2025

This was the first try to get your opinion on the proposed changes. I am ready to complete all the test classes of course.
I returned assert back for assertions.

Great! Thanks. Please, ping me here when the remaining tests will be ready.

@mipo256 mipo256 self-assigned this May 29, 2025
@mipo256 mipo256 added the enhancement New feature or request label May 29, 2025
@ignatmaloukhov ignatmaloukhov requested a review from mipo256 July 3, 2025 04:30
mipo256
mipo256 reviewed Jul 6, 2025
View reviewed changes
src/test/java/org/editorconfig/plugin/maven/model/EndOfLineTest.java
@ParameterizedTest
@EnumSource(value = EndOfLine.class, names = "CARRIAGE_RERUN_LINE_FEED", mode = Mode.EXCLUDE)
void isSingleCharacter_true(EndOfLine source) {
void isSingleCharacter_СrLfEof_True(EndOfLine source) {
Copy link
Member

Choose a reason for hiding this comment

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

It is everything except the CRLF, which is not a single character, so the name of the test is misleading :)

mipo256
mipo256 requested changes Jul 6, 2025
View reviewed changes
Copy link
Member

@mipo256 mipo256 left a comment

Choose a reason for hiding this comment

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

In general, everything is alright, The only thing I would suggest is to separate the pom.xml changes from the refactoring in tests into two separate commits

@mipo256
Copy link
Member

mipo256 commented Jul 18, 2025

Hey @ignatmaloukhov!

Are you on track? Shall I take over if you're limited on time?

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

Reviewers

@mipo256 mipo256 mipo256 requested changes

Assignees

@mipo256 mipo256

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adopt a single naming convention in unit tests

3 participants

@ignatmaloukhov @mipo256 @imalouhov