ebshimizu / hots-parser

A node-based Heroes of the Storm match parser
MIT License
12 stars 3 forks source link

Tests #5

Closed jnovack closed 6 years ago

jnovack commented 6 years ago

Can you please upload a test replay (test/replays/example.StormReplay), and test json output (test/example.json) so that I can write tests around it.

This replay and example would be the basis of moving to v6.0.0 so please export them from the current production desktop application so I can ensure this repository it is a field-for-field match of JSON.

ebshimizu commented 6 years ago

towers_ref.zip

Towers of Doom Unranked Draft. Output json file is the direct dump of the object returned from processReplay() from version 6 of the parser.

Big caveat on one of the fields: match.objective is different for every single map in the game. I am open to fixing this in a later version (it's a non-trivial change in the app at the moment but I realize having different schemas for 13+ maps is Not Great).

Other notes:

jnovack commented 6 years ago

I did have to load a heroes-talents instance for testing.

That may need to be it's own module if you want automated testing. It can be written to download the github repo and process the files automagically just like heroprotocol.js does with the protocols.

  parser.js
    ✓ parser version 6
    ✓ json deep equal (1144ms)

  2 passing (1s)
jnovack commented 6 years ago

https://www.npmjs.com/package/heroes-talents

Just quickly made that. I'm sure there are files in the wrong place. but it was a quick method to get it as a package that automatically downloads the latest repo files.

ebshimizu commented 6 years ago

I thought I wrote heroes-talents out of the parser though. The signature of processReplay looks like https://github.com/ebshimizu/hots-parser/blob/399dec5024bb906c95739522d0c4279ad4bcd5d3/parser.js#L145 in the current version, so the heroes talents instance shouldn't be needed anymore (for the parser, in general it's a super useful package).

I can also provide replays for each map spread across all of the supported game modes for testing.

jnovack commented 6 years ago

I didn't change any code within parser.js (just console.log), so I'm confident it works exactly the same.

I just wanted a control known-good, so that when we DO (if ever) have to modify the parser, we know we are not breaking anything.

For today, I wanted YOU to be confident in the package. For tomorrow, I wanted any changes we make to parser.js to against a known good while still on SCHEMA 6.

I thought I wrote heroes-talents out of the parser though.

You did, I didn't bother testing without it. I can do that tonight and submit the next patch release.

ebshimizu commented 6 years ago

Ah got it. I would like to refactor the parsing code itself at some point (it was written rather quickly) so I very much appreciate the addition of some actual tests :)

I'll keep an eye out for the update to the PR. Thanks for helping out with this!

jnovack commented 6 years ago

I thought I wrote heroes-talents out of the parser though.

You did remove it, and it does pass the tests without it. I'll remove it from the hots-parser repo, BUT heroes-talents is still useful, that's another drop-in replacement for your electron app. Also, as long you farm out pieces of the electron app as modules, I can modify them and make feature parity for the webapp version. :)

jnovack commented 6 years ago

Pretty sure this was completed with merge.