test: automate netregex tests #138
Conversation
github-actions
bot
added
docs
| ], | ||
| }, | ||
| unitTests: [ |
There was a problem hiding this comment.
I don't really agree with adding the unit test info to this file. This feels like a separation of responsibility issue, e.g. why should the built and deployed to users code include unit tests?
Maybe this requires a separate test/helper/example_log_lines_test_map.ts file or something.
There was a problem hiding this comment.
I don't really agree with adding the unit test info to this file. This feels like a separation of responsibility issue, e.g. why should the built and deployed to users code include unit tests?
Maybe this requires a separate
test/helper/example_log_lines_test_map.tsfile or something.
If the concern is including this in the deployed code, then the entire file could be moved to util/, since its only uses are for util/gen_log_guide and the unit tests. The file isn't used as a runtime resource, so maybe that's better anyway. (Edit: Or I suppose it could be moved into test/helper/ and the log guide script could read from there. The example lines and unit tests are stored together right now in the test files, so that's closest to what we have now.)
I think I would prefer that approach over splitting the unit tests from the example lines because they are closely related (same data, and connected via indexToTest), and I think it's helpful to be able to see and update example lines and the unit tests that use them in a single place. Putting them in different places means yet another file to update when log lines are added/changed + the need to keep the properties in sync. And the unit tests need to read the example lines too.
There was a problem hiding this comment.
I don't really agree with adding the unit test info to this file. This feels like a separation of responsibility issue, e.g. why should the built and deployed to users code include unit tests?
Been giving this further thought, and I'm okay with separating the unit tests from the example line data. I think the case could be made either way for viewing this as test data that's exposed in LogGuide, or LogGuide-supporting data that we happen to also test against. At the end of the day, regardless of implementation, Typescript will throw compile-time errors if log def types are added without both example lines and unit tests, and mocha will report mis-matched unit test data, and since those are the primary concerns, anything else is just convenience.
I am going to move the example lines to util/ though, because there really isn't a need for it to be in /resources. I'm also going to separate out the test data and make the necessary changes in #139 rather than here, because there were additional structure changes there that make rebasing/resolving merge conflicts needlessly bloody. (Yes, I could've avoided that by waiting to PR #139 until this was merged, but oh well.)
…e example lines (via LogGuide) with inaccurate fields
There was a problem hiding this comment.
Pending moving example lines to util as mentioned in https://github.com/OverlayPlugin/cactbot/pull/138/files#r1582534407 as well as the potential unintended change mentioned below, this looks good to merge.
| '262|2023-04-21T23:24:06.0630000-04:00|en|00000051|_rsv_35827_-1_1_0_0_S13095D61_E13095D61|Further testing is required.�����,\\r���)������ ��, assist me with this evaluation.|38151741aad7fe51', | ||
| '262|2023-04-21T23:24:06.0630000-04:00|en|00000051|_rsv_35827_-1_1_0_0_S13095D61_E13095D61|Further testing is required.�����, ���)������ ��, assist me with this evaluation.|38151741aad7fe51', |
There was a problem hiding this comment.
This seems like an unintentional change? Did you verify this from raw log line info?
There was a problem hiding this comment.
This seems like an unintentional change? Did you verify this from raw log line info?
I originally took it out because it was causing issues with the unit test, and it didn't seem worth it to write logic just to handle special characters solely in a unit test example that we ourselves are providing. (As an alternative, we could just replace this line with a unit test that doesn't have special characters). I probably should add a tracking issue to make sure the special character problem is limited to unit testing and not actually a live capture issue.
Might've gotten lost in my comment above, but:
Admittedly a self-inflicted wound, but I'd prefer not to do the file move & separate out the test data in this PR, only to then have to resolve a bunch of conflicts in #139; the last commit in #139 already handles the file move & data separation. If you think that's a problem, the maybe-cleaner other option is just to close this PR and handle everything in #139, since it already includes all the changes in this PR. They were originally separated for reviewability, but since you've already looked at both, JLMK what you prefer. |
|
Changes addressed in #139 - closing/superseded. |
More work on #125 .
1fa2945includes all of the changes made in #137 , so can be ignored here. (I will rebase it out once 137 is merged).This also fixes a few missing/incorrect fields in
netlog_defs, and replaces a handful ofLogGuide.mdexamples that were out of data and would no longer be parsed correctly due to missing/misordered fields. [NB. One incidental benefit of this change is that theLogGuideexamples will now (need to) be kept up to date if there are changes in field defs or regex, since the examples will always be parsed using current regex structure).I'm going to handle the changes to
regex_test(ACT line format) last, but it should be fairly small and mostly deletions, since this PR has most of the groundwork. I just didn't think it made sense to pile anything else into this PR.