search

Ostoic / RaidBrowser

Bringing LFR to Wrath of the Lich King (Non-Classic)
MIT License
36 stars 19 forks source link

DRAFT: Improve support for Weekly Quest and Achievement run postings #37

Closed MassCraxx closed 1 year ago

MassCraxx commented 1 year ago

I noticed only some of the often posted weekly quests and achievements are being parsed, so I put some work into improving this.

These changes will also support localized clients, since both, english quest/achievement title and actual ID in links are compared. While working on this I also noticed that LF5M or alike are not being processed, so that is fixed as well.

improve parsing weekly quest raid messages, add new category for weekly quest raids (wq), improve parsing achievement quest raid messages, improve parsing lfm (including LF5M etc), add some missing dps patterns

Ostoic commented 1 year ago

I'll have to test this one out since there are a lot of additions to parsing rules being made. Looks great, though.

MassCraxx commented 1 year ago

Yes, we should test this thoroughly, I keep finding border cases.... Did you know its not possible to post a NM achievement if you have the according HC one already? So a lot of ppl post "LFM icc10nm [Heroic: etc]" ... which would currently be seen as icc10hc, but I may have found a solution for this now

Ostoic commented 1 year ago

If you have a Lua notepad addon or something of the sort, you can run the lfm_tests.lua file. The idea is to have at least some degree of unit testing involved in development, especially because of the indeterministic way in which parsing is done and the potential for edge cases.

I've noticed the difficulty vs. achievement discrepancy in the past before. There's a unit test case that covers something similar.

Ostoic commented 1 year ago

There are 25 extra failed unit test cases, so I'll have to look into this a little more. I'd like to avoid any regressions if possible.

MassCraxx commented 1 year ago

I use Visual Studio Code, I'm sure its possible to integrate a lua runner somehow, but I did not put time into this as of yet... So I will just call your tests from in game ^^. Its really awesome that you have these tests! I would have never found all the problematic cases otherwise. I already had a quick look on fixing these cases and will come back to you once the failed tests are minimal. This may however lead to even more and more in-depth changes...

Ostoic commented 1 year ago

Yeah I figured it would be sort of impossible to ensure some degree of correctness from release to release. I was thinking the same sort of thing in terms of running the tests in vitro. Ideally, we just need a few things to mock the covered WoW API calls, but this might be more work than it seems. Good to hear though!

MassCraxx commented 1 year ago

Ok, sorry for the upranking complexity of changes, but I got it down to 4 failed test cases, and I think these remaining ones are questionable:

I think the changes are quite solid, but I am not sure if the finding process can be simplified: for my current approach I first look for the pattern index before replacing the pattern with the meta placeholder (core.lua L1012+1015) I was thinking maybe both can be done in one action? Let me know what you think.

Ostoic commented 1 year ago

I like the idea about putting weekly quests into their own parsing raid category. I also like the new debugging functionality.

Yeah those test cases (aside from the last one) are questionable and can probably be removed. Ideally, it would be nice to try to classify the current test cases to discover new classes of test cases to consider. I think I might look into creating a test environment so that we could see code coverage, and in particular, lfm pattern coverage. We could also probably quickly do this manually, so I might just do that instead for now lol.

I've been testing the changes and everything seems to be working well. I indeed get a 4/198 failed (pretty much 1/198) rate as you said, which is great!