search

kaitai-io / kaitai_struct

Kaitai Struct: declarative language to generate binary data parsers in C++ / C# / Go / Java / JavaScript / Lua / Nim / Perl / PHP / Python / Ruby
https://kaitai.io
3.92k stars 193 forks source link

`CastToImported` and `CastToImported2` tests are still shows in the dashboard after them was removed #1114

Open Mingun opened 3 months ago

Mingun commented 3 months ago

The spec cast_to_imported.kst was removed in https://github.com/kaitai-io/kaitai_struct_tests/commit/83624da6cb4ed2047c85346f6e7a97bead8e4a93#diff-645011c80deef943326f24c0cbf570ba4fcb97b7627bb89ea2fd22d4fb9f4ea1 (2024-04-01), but is still presented in list of tests in https://ci.kaitai.io/ and even marked as passed for C++ and graphvis. The dashboard was updated 2024-04-03 after that deletion has been performed. So the question, why that results are still there?

generalmimon commented 1 month ago

Moved from https://github.com/kaitai-io/kaitai_ci_ui/issues because this issue effectively doesn't point out anything wrong in https://github.com/kaitai-io/kaitai_ci_ui - the CI dashboard app only presents test result data, but this is an inconsistency in test result data, i.e. in the overall "testing infrastructure".

even marked as passed for C++ and graphvis

No, only for GraphViz. In C++, the generated source files corresponding to cast_to_imported{,2}.ksy failed to build (cast_to_imported.cpp errors, cast_to_imported2.cpp errors), so that's one reason why their rows are displayed. Another reason is that GraphViz reports them as passed, because they're not explicitly excluded in TextListParser (BTW, this BLACK_LIST is IMHO quite well hidden, I just stumbled upon it relatively recently).

Ideally speaking, I think (probably like you) that in the end CI dashboard shouldn't display rows that don't represent actual tests (what this really means is that they should be omitted from ci.json files, not just "hidden" by any CI dashboard display logic which doesn't have enough information to decide that anyway). But the detection of real tests should be automatic, the BLACK_LIST in TextListParser is a terrible idea (we can see how it doesn't work at all in the long run, because no one ever remembered at the right time that this list exists and needs to be updated). Unfortunately, our testing system doesn't allow for easy detection whether a generated source file corresponds to a test or not (a naive idea might be to check whether the relevant spec/ks/{name}.kst file exists, but we have tests that don't use .kst but are instead implemented manually in unit test specs for each target language, because they need to use asserts that cannot be expressed using KST).