Add replay file tests

[redraskal]

We should create some tests to read a few replay files and validate the data.

This would help test compatibility between multiple versions of Siege

[stnokott]

I'm preparing a merge request for this.

The main focus of this is backwards compatibility, right?

Since we would probably use the current version once to dump a replay file as JSON, then write a test that asserts that future code bases produce exactly the same JSON, this would be a test that fails as soon as you restructure your structs or the JSON output struct, so possibly very often.

Meaning, when you change the structure of any of the exposed structs, the test will fail, because the expected JSON is not 100% equal to the actual structure.

[redraskal]

Yeah, tests may not work well if we look for an exact match. I think naming conventions are in a good place right now for the output.

Perhaps, ensure

1. The key-value pairs present in both dumps match (ex. old dump has map key, new dump has map key- value should match)
2. Keys were not dropped in the new dump (if map was present in old dump, must be present in new one)

This should tell us when we break compatibility and when we break a feature.

[redraskal]

I think this design would avoid frequent fails, thoughts?

[stnokott]

I'm unsure. Restricting ourselves to only validate existence of specific keys would raise the question of which keys we deem required (should be tested) and which not (should not be tested). When looking through the current JSON, I don't see any fields that I would deem "unnecessary", thus I would want to test every key for a match. And at this point, we can just test the whole JSON.

Looking at it now, I think frequently failing tests wouldn't be a bad thing (Fail-Fast = good). If a test fails, that means that the way the program generates its output changed which indicates a breaking change. I think that's a good thing.

I'd say if you think the naming conventions are in a good place and won't change much in the future, then the best solution would be to split the JSON dumps in multiple parts according to the output struct: https://github.com/redraskal/r6-dissect/blob/02f9437e8bb969e0d7d88417af0fe3cf85721456/main.go#L148-L152 And then test each part individually for exact match.

[redraskal]

Mostly concerned about new features down the line. The test would fail from new information (unless its new Siege content). Wouldn't we need to clear the old dumps every time we add to the Header?

[redraskal]

Seems like that could overshadow something actually breaking if we had many replay files to test

[redraskal]

I guess my idea could be simplified to checking if the difference between both dumps are only additive (or exact match)

[stnokott]

Since these tests would not really serve to validate program behaviour, but instead serve as a regression test notifying us of any changes to the output JSON format, I think adding new information should fail the tests. It would allow us to check again if the new format looks good, change the expected test data accordingly and then move on.

I get your point, but if we were to write "additive" tests, we would be able to continue adding information to the output JSON without ever changing the expected JSONs. Exaggerated, we could have empty expected JSONs and our tests would still succeed.

[redraskal]

I understand your point, I think your approach makes sense.

[stnokott]

Ideally, we wouldn't even need to focus on this much if we would write unit tests. I find that hard with this project though since our only input is a file, making dependency injection tough.

[stnokott]

As mentioned, I'm currently trying to write up a few regression tests. I am struggling to unmarshal the exported JSON back into structs due to some of the json.Marshaler implementations. This method for example: https://github.com/redraskal/r6-dissect/blob/02f9437e8bb969e0d7d88417af0fe3cf85721456/dissect/feedback.go#L38-L40 We lose all information about the underlying int value of MatchUpdateType. Do you have an idea on how to solve this issue, ideally without having to change the implementation?

The only solution would be to not store our expected JSONs in the format used by the export function, but instead define our own structs only used for testing. Would be more maintainability overhead, but would allow us to test with more granularity.

What do you think?

[redraskal]

I noticed this also caused problems with generated types from Wails. I have not yet looked into a solution for it.

That is fine with me

[stnokott]

I noticed this also caused problems with generated types from Wails. I have not yet looked into a solution for it.

In relation to #9 ? Wails currently has some issues with Typescript generation, if that's what you mean. It's not necessarily related to your code:

• https://github.com/wailsapp/wails/issues/2348
• https://github.com/wailsapp/wails/issues/2323
• https://github.com/wailsapp/wails/issues/2303
• https://github.com/wailsapp/wails/issues/2258
• ...

[redraskal]

Yes & true- could be unrelated.

[stnokott]

Some tests are now here: https://github.com/stnokott/r6-dissect/tree/compatibility-tests Waiting for #40 to be resolved before opening PR.