search

open-obfuscator / o-mvll

:electron: O-MVLL is a LLVM-based obfuscator for native code (Android & iOS)
https://obfuscator.re/omvll
Apache License 2.0
617 stars 66 forks source link

Add copy of Python-3.10.7/Lib as test input to fix find-omvll-config #23

Closed weliveindetail closed 6 months ago

weliveindetail commented 1 year ago

These tests failed when running the test suite without OMVLL_PACKAGE set:

In both cases Python scripts failed to execute, because the fake Python Libs folder didn't have the stdlib files. The same thing happens on Linux. I must have missed to run them from a new terminal last time (one that doesn't have OMVLL_PACKAGE set).

The only way I see to fix this quickly is to add the Python Lib contents as test input. This is 10M of text files, so this is not ideal. If we had more time we could check which of the files are actually used. (In the end all we run is an almost empty Python script.) Or should we download it at configuration time? Or maybe we add it as a resource to the plugin binray at some point? We will see. For the moment, I think this is acceptable. Eventually, we only every link a single Python version.

The review of the diff isn't ideal either. I don't see how to hide new files in the diff here on GitHub. The updated tests can be checked here as well: https://github.com/open-obfuscator/o-mvll/tree/9972f155e438a6826201c279672888e55b896a0a/src/test/core/plugin

sevilS commented 6 months ago

Hi @weliveindetail , I am taking a look at this, in order to merge it with https://github.com/open-obfuscator/o-mvll/pull/40

I am a bit confusing because of the following, I am able to execute the tests if the OMVLL_PYTHONPATH is set and this mentioned variable OMVLL_PACKAGE is not set, in my macOS, without doing any change on main branch

If I unset OMVLL_PYTHONPATH and comment the requirement, the tests are not working, but I believe there is no point to not have this variable set, isn't it? Additionally if I apply you two fixes on the files, 2 out of 11 tests are working only, so still missing something.

I believe I am missing something you may have clear, can you clarify it for me? Maybe this is something that was happening but not anymore?

Thanks, Sergi

marcobrador commented 6 months ago

Running the tests does require setting OMVLL_PYTHONPATH env var. Closing this as not relevant anymore.

weliveindetail commented 6 months ago

Hi @marcobrador and @sevilS

Sorry for the delay, I was just taking a look today. It's been while, so I had to reproduce the tests myself and revisit the context.

TL;DR: Closing this is the right thing to do, but it might be worth thinking about a strategy for testing release packages. Let me still follow-up on your questions.

I believe there is no point to not have this [OMVLL_PYTHONPATH] variable set, isn't it?

This is correct, if we want to run the tests against the build tree (like in any regular CI testing). However, I think the idea at the time of this PR was to run the regression tests on the MVP package (like in release testing). I think this is where the OMVLL_PACKAGE variable originated from, but I don't find any reference to it in the sources. Did we have a ticket for this maybe?

I believe I am missing something you may have clear, can you clarify it for me? Maybe this is something that was happening but not anymore?

IIRC omvll.yml provides exactly these two pieces of information and by setting them both upfront, we circumvent the whole process of finding omvll.yml. This is, however, a pretty critical aspect of a release test, especially since there are two distinct approaches right? From what I remember, we find omvll.yml by traversing parent directories from: (1) CWD when testing development builds (it's somewhere in the build-tree) (2) the plugin's directory in release builds (it's part of the MVP package)

Additionally if I apply you two fixes on the files, 2 out of 11 tests are working only, so still missing something.

So, having this reconstructed, it actually makes sense. The two tests that pass are the ones to find omvll.yml and thus cover the code for the above case (2) 🙂

PASS: O-MVLL Tests :: core/plugin/find-omvll-config-cwd.c (8 of 9)
PASS: O-MVLL Tests :: core/plugin/find-omvll-config-plugindir.c (9 of 9)

I guess back then this was "good enough" for the moment and we got derailed by higher-prio issues. Maybe we didn't get to the remaining infra for MVP package testing and this mysterious OMVLL_PACKAGE approach? Well, looking back today, it might have been worth spending at least the time to explain the context in more detail in the PR.