intel / hdk

A low-level execution library for analytic data processing.
Apache License 2.0
31 stars 14 forks source link

Refactor tests #686

Closed leshikus closed 1 year ago

leshikus commented 1 year ago

Subj

leshikus commented 1 year ago

@alexbaden there are multiple matters

1) the recent version of conda run has a bug that it can be broken by some specific option even if this option is a part of a command we want to run; thus I moved commands into dedicated scripts; this also adds to readability

2) I wanted to remove code and build duplication for some time, so I finally made this along with the previous change

3) I combined pytest and modin tests into one task; this is just a matter of taste; we may discuss further the name for a combined task or I can revert the change; the tests from both pytest.yml and modin.yml were moved as is into test-modin.sh with the following exception (4)

4) I removed duplicated runs for python python/tests/test_pyhdk_bindings.py and python python/tests/test_pyhdk_sql.py; actually it was me who added them on Igor's request before pytest.yml was created so I have a responsibility to remove them if they are no longer needed; if they are needed I'll add them back, they run second time in a slightly different environment with modin installed;

5) I replaced "Gtests (Sanity)" with "Gtests" everywhere - this is also subjective and my variant is shorter

alexbaden commented 1 year ago
  1. I combined pytest and modin tests into one task; this is just a matter of taste; we may discuss further the name for a combined task or I can revert the change; the tests from both pytest.yml and modin.yml were moved as is into test-modin.sh with the following exception (4)

I see... the file should probably be renamed, then.

Regardless - to me, this is a refactoring of existing jobs that is not strictly better than what we had before, with no functional changes (except perhaps merging the HDK + Modin tests, which is still a little unclear - @Garra1980 thoughts?). I also would tend to disagree that moving parts of the test to bash scripts improves readability. To me, it means I need to now look in two places to see what a test is doing, instead of just one.

leshikus commented 1 year ago

@alexbaden that's a good question about readability, let me share my thoughts

1) the idea is to combine multiple lines of code into functional unit which have a descriptive name; 2) replacing two 200+ line files with two ~10 line files improves things as well 3) some command lines, style check in particular, are two long compared to generic lines

while readability question is subjective, another point has a stronger justification; we know now that conda run command line is buggy (-vx case), thus we need to keep it minimal

example: python style check didn't look better than c++ style check; I kept it intact because there was no conda run there

leshikus commented 1 year ago

@alexbaden let me elaborate that second reason a bit

if we will use conda run -n omnisci-dev sh script_name.sh in most places, we ensure their command line parser have less chances to parse our directives incorrectly

leshikus commented 1 year ago

@Garra1980 told me, "I prefer not to change anything"; closing this PR