-
Notifications
You must be signed in to change notification settings - Fork 230
Add Dota 2 VideoGameProvider #1315
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
Conversation
PR Summary
|
kingthorin
left a comment
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.
LGTM
kingthorin
left a comment
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.
Thanks!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1315 +/- ##
============================================
- Coverage 91.82% 91.75% -0.07%
- Complexity 3086 3094 +8
============================================
Files 310 311 +1
Lines 6078 6090 +12
Branches 631 631
============================================
+ Hits 5581 5588 +7
- Misses 342 344 +2
- Partials 155 158 +3 ☔ View full report in Codecov by Sentry. |
|
I was going to suggest something like: @Test
void dota2Quote() {
// Given / When
String quote = dota2.heroQuote(dota2.hero());
// Then
assertThat(quote).isNotEmpty();
}but it seems there aren't quotes for every hero. I guess you could make a list of those that do have quotes and then randomly pick from that Set for the test. |
|
Also, in the future, when quotes are added for all heroes from the game itself, we should get rid of “availableHeroes” in Dota2Test itself and change the dota2HeroQuote test method to take heroes from dota2.hero(). |
|
@bodiam how sticky (or not) do you want to be about the codecov stuff? |
|
@kingthorin not very, it was more indicative than anything else. I'm not sure why it breaks the build these days when the coverage goes down a bit, that wasn't intended. |
|
Thanks. Merging as-is 🙂 |

All data was taken from open sources (like dota2.fandom.com).
There was already a dota.todo.yml concept, I changed the data and added the data in the afternoon, added a couple more methods to the new provider itself