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

Refactor HarvesterOptionsTest to use assertj #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eperott
Copy link

@eperott eperott commented Mar 30, 2020

Also, moving test class into the correct module.

Also, moving test class into the correct module.
@eperott
Copy link
Author

eperott commented Mar 30, 2020

Proposing to make test asserts with assertJ style, as it generally improves readability. Of course, this is a matter of opinion/preference.

And speaking of preferences, should we use Test prefix or suffix in test class names in cassandra-exporter. I'm proposing to use TestMyClass (over MyClassTest). In my experience this will save me a couple of key strokes every time I'm looking up a class in IntelliJ (searching with ctrl+n).

Lot's of opinions here. :-) Let me know what you think.

@smiklosovic
Copy link

I prefer MyClassTest but if we want to change it I dont mind however we should be consistent so we should rename all of them. MyClassTest is more readable in my books as I just do not need to parse in my head what test it is after that Test prefix but only my preferencies speak here ...

@eperott
Copy link
Author

eperott commented Mar 31, 2020

Agree, being consistent is the most important thing. Which is why I brought it up - currently we're not.

Whether we go for prefix or suffix is not something we need to debate at any length. You guys, being maintainers, should probably just decide which way to go.

@johndelcastillo johndelcastillo force-pushed the master branch 2 times, most recently from 3b88272 to 84cb7cd Compare October 19, 2023 03:44
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