-
Notifications
You must be signed in to change notification settings - Fork 176
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
Apply comparison against yaml file content for tests within entertainment package #677
Conversation
@@ -1,19 +1,19 @@ | |||
package net.datafaker.providers.entertainment; | |||
|
|||
import org.junit.jupiter.api.Test; | |||
import net.datafaker.providers.base.GratefulDead; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be moved into the entertainment package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
done, thanks
@Test | ||
void wookieWords() { | ||
assertThat(faker.starWars().wookieWords()).isNotEmpty(); | ||
void alternativeSpelling() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can’t be done the new way? (I’m on my
Phone and couldn’t check the code or data file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some methods in different classes including this one which are a bit trickier under the hood...
e.g. this method looks like
public String alternateCharacterSpelling() {
return resolve("star_wars.alternate_character_spellings." + CHARACTERS[faker.random().nextInt(CHARACTERS.length)]);
}
so it requires some more sophisticated handling...
I would rather migrate everything that's possible to new way and then try to handle the rest
Thanks for tackling this FYI any tests in the base package that used regex I just skipped over. I’ll have a better look at them and decide if others should be moved to the new infrastructure. |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #677 +/- ##
============================================
- Coverage 92.83% 92.76% -0.08%
+ Complexity 2647 2645 -2
============================================
Files 285 285
Lines 5445 5445
Branches 590 590
============================================
- Hits 5055 5051 -4
- Misses 241 243 +2
- Partials 149 151 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
yes, some of them are tricky... |
The PR applies comparison against yaml file content for tests in entertainment package.
P.S. Probably later will do for others