-
Notifications
You must be signed in to change notification settings - Fork 8
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
Question about testing #3
Comments
@lwojcik I didn't get really started on the testing, so feel free to take whatever approach you think is best and then I will just jump in from there boss! |
Roger that! |
@lwojcik I'd hold off on the testing for a couple of days. I am doing a large overhaul of adding in type declarations for all of the return types for all of the game functions. So, instead of Having this additional information should make for more thorough testing. |
All right, I'll wait. Let me know when you're ready. |
Hey there @lwojcik! I have managed to get the type declarations done for both D3 and Hearthstone. The changes aren't in master yet as I am waiting to get all of the games done before the merge. That being said, you could branch off of my PR branch here: #4 and start working on the testing for the D3 and Hearthstone bits. At least that gives you a place to start if you have the free time right now! Thanks, Shane |
All right! I'll try to start next week. |
Hi!
Tonight I sat down to read your code and try writing some unit tests. It was a definitely a fun evening but not particularly productive so far, as I had to deal with some cryptic errors that didn't seem to be directly connected to your code. I also noticed you test dist build rather than TypeScript files.
I wonder if testing the dist build was a deliberate choice of yours as I'm tempted to follow the approach to unit testing I use myself, i.e. add ts-jest and reconfigure unit tests to test source files directly rather than generated *.js files, thus eliminating errors caused by build dependencies. It makes coverage reports easier to read and seems to be a sane approach, but it's rather a big change and I'd like to read your opinion first.
Are you okay with changes I mentioned above?
The text was updated successfully, but these errors were encountered: