Add faker for pulling unique values from files #296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
============================================
+ Coverage 94.89% 94.93% +0.03%
- Complexity 1916 1928 +12
============================================
Files 201 202 +1
Lines 3823 3853 +30
Branches 379 383 +4
============================================
+ Hits 3628 3658 +30
Misses 99 99
Partials 96 96
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| if (!(object instanceof List)) { | ||
| throw new RuntimeException(String.format( | ||
| "Object found for key %s is not in list format", |
There was a problem hiding this comment.
In fact this exception text will not help end user to understand what he/she has done wrong
There was a problem hiding this comment.
I think this issue has been resolved. I completely removed that exception and reused the "No values found for key" exception if it failed to convert to a list.
| )); | ||
| } | ||
|
|
||
| return new ArrayList<>((List<String>) object); |
There was a problem hiding this comment.
There is no check that it is a list o strings and not a list of lists....
So probably in case it is a list of lists it makes sense to throw an exception like there is not value for such a key
There was a problem hiding this comment.
I added a check that filters out any lists and throws the same "No values found for key" exception. Is RuntimeException okay for that or is there a more suitable exception for this case?
|
Thanks for the contribution, I left some comments. |
|
Thanks for the feedback! I do have one additional question. Since it's possible for YAML files to contain other data types such as integers, how should those be handled? Is it okay if we just return them as strings, or should they be handled differently? |
this is a good question... currently i do not have a good answer for that... |
- Change exception types and messages - Change way value is removed from list to be more efficient - Add stricter type checking for objects found in yaml files Refactor UniqueTest to use less mocking Handle other data types in fetchFromYaml method
|
Along with the other fixes, I updated the fetchValues method to automatically convert non-string values into strings. It didn't looks like there were many file based fakers that returned something other than a string. I think if we want to account for other datatypes we could add another method like fetchIntegerFromYaml. Let me know if there are any issues with that. |
- Change all exception type to NoSuchElementException - Replace private method for swapping indexes with Collections.swap() - Remove try catch block for catching type conversion issues
| import java.util.*; | ||
|
|
||
| import static java.util.Arrays.asList; | ||
| import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
please use assertJ assertions like in all other tests instead of junit's
There was a problem hiding this comment.
This should be resolved now.
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class Unique extends AbstractProvider { |
There was a problem hiding this comment.
Would it be possible to add a bit of documentation to the class and maybe public methods? It's a little light on documentation as it stands, which makes it a bit harder to use for some people (I hope I'm not asking too much, but some documentation to the Markdown docs on how to use / purpose would be great too!)
There was a problem hiding this comment.
I added some brief documentation to the class itself and the method. Let me know if anything can be phrased differently or if more explanation is necessary. I'll also work on getting some actual documentation added into the markdown docs.
|
Hi all, what's the state of this PR? I see a few open remarks related to the testing, but I don't think they're blockers for including this PR. Is there anything missing? I'd love to make a new release this week or so, but I'm not sure if this one should be included too? |
|
I just pushed up a couple commits that should resolve most of the outstanding issues. Assuming that those commits look good, the only thing that needs to be added are markdown docs. If those are necessary for this PR to be part of the release I can try to get those done this week. |
|
Great, thank you. While the markdown documentation would be a nice to have, we can always add it later in a separate pr. It just helps people to find features like this a bit easier I guess. Thanks again! |
This PR should solve issue #265 by pulling a list of all values for the given key, and popping off a random value every time. Please let me know if there are any issues with the PR.