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: Use Standard Tech for Test Fixtures #3551

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wadoon
Copy link
Member

@wadoon wadoon commented Feb 15, 2025

This PR removes the LineProperties.java in favor of YAML test fixture.

Pros:

  • Support by JUnit YamlSource in parameterized test
  • Support for YAML in browser
  • Flexibility and type safety

This PR brings two dependencies for tests:

  • AssertJ for writing simpler assertion in test with better messages.
  • Jackson for reading YAML

@wadoon wadoon requested a review from mattulbrich February 15, 2025 01:05
@wadoon wadoon self-assigned this Feb 15, 2025
@wadoon wadoon added the 🛠 Maintenance Code quality and related things w/o functional changes label Feb 15, 2025
@mattulbrich
Copy link
Member

Unfortunately, this PR does not follow the PR summary template of the project. Can you please provide the information of the template?

@mattulbrich
Copy link
Member

Thanks for replacing the parser. Actually, I think it is more understanable and clearer with the test cases in individual files. Can that be arranged?

@mattulbrich mattulbrich added the Reviewer Feedback Feedback from the review needs to be addressed label Feb 22, 2025
@wadoon wadoon force-pushed the weigl/cleanup/yamlfixtures branch 2 times, most recently from d21e198 to 23a566f Compare February 23, 2025 20:30
@wadoon
Copy link
Member Author

wadoon commented Feb 23, 2025

Thanks for replacing the parser. Actually, I think it is more understanable and clearer with the test cases in individual files. Can that be arranged?

done

@wadoon wadoon force-pushed the weigl/cleanup/yamlfixtures branch from 23a566f to 334dead Compare February 24, 2025 00:41
@mattulbrich
Copy link
Member

Thanks for replacing the parser. Actually, I think it is more understanable and clearer with the test cases in individual files. Can that be arranged?

done

thanks. Is it on purpose that the files are in a different directory now?

@wadoon wadoon force-pushed the weigl/cleanup/yamlfixtures branch 2 times, most recently from d883e5e to 1227678 Compare February 25, 2025 17:42
@wadoon
Copy link
Member Author

wadoon commented Mar 5, 2025

thanks. Is it on purpose that the files are in a different directory now?

Avoidance of one more folder level in the hierarchy. It seemed to be unnecessary for script fixtures.

@wadoon wadoon force-pushed the weigl/cleanup/yamlfixtures branch from 1227678 to a91c56f Compare March 5, 2025 22:10
@wadoon wadoon added this to the v2.12.4 milestone Mar 5, 2025
@wadoon wadoon added Review Request Waiting for review and removed Reviewer Feedback Feedback from the review needs to be addressed labels Mar 5, 2025
@mattulbrich
Copy link
Member

I have little experience with yml, and, hence, it seems to me that this change actually adds complexity rather than reducing it. I cannot appreciate the improvements that this PR bring. Perhaps someone more familiar w/ the language can see this and do the review.

@mattulbrich mattulbrich removed their request for review March 11, 2025 16:23
@wadoon wadoon force-pushed the weigl/cleanup/yamlfixtures branch from 5532bd2 to eaa98f8 Compare March 16, 2025 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠 Maintenance Code quality and related things w/o functional changes Review Request Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants