Fix path comparison

[sschulz92]

I identified an issue using relative imports. After several iterations I found out, that the method get_all_methods_in() was comparing file paths, which were equal except the C: upfront. One path was using C the other one c and therefore no step was found.

At runtime I could see the following error:

Failed Step: Start polling <step_var>
Specification: <PATH_TO_SPEC_FILE>.spec:31
Error Message: PollingSteps.start_polling() missing 1 required positional argument: 'step_var'
Stacktrace: 
Traceback (most recent call last):
   File "<PYTHON_PATH>\site-packages\getgauge\executor.py", line 35, in execute_method
      step.impl(*params)
     ~~~~~~~~~^^^^^^^^^
TypeError: PollingSteps.start_polling() missing 1 required positional argument: 'step_var'

The mentioned class is required to be instantiated. Therefore we had 2 positional arguments: self + step_var! Since self should have been gone at this point already, the issue appeared.

Overall I think the issue is, that we mix up different path variables. If you would use paths from pathlib everywhere, the issue should have been resolved in first place. To resolve my issue, the str().lower() approach is easier though.

After installing a custom python version on my machine, the test scenario is running fine again!

[sschulz92]

@zabil @sriv @chadlwilson someone in the position to review it? 😄

[zabil]

Not a full review. But won't this cause an issue on non windows system where the filenames and paths are case sensitive?
Is it possible to write a test (to simulate the error) and to make sure this is cross platform?

[sschulz92]

Not a full review. But won't this cause an issue on non windows system where the filenames and paths are case sensitive? Is it possible to write a test (to simulate the error) and to make sure this is cross platform?

Fair point, I was not aware of this fact. I will have another look!

[chadlwilson]

Not a full review. But won't this cause an issue on non windows system where the filenames and paths are case sensitive?

This was my first thought after skimming through this, but didn't have time/energy to think it through and write those tests myself.

It usually ends in pain to try and use magic to treat case sensitive file systems as if they are non-sensitive, and vice versa. And there are weirder cases for in-between, case-preserving file systems.

Is it possible to write a test (to simulate the error) and to make sure this is cross platform?

Yeah, I found it difficult to follow which problem is being attempted being fixed here without tests of any kind.

[sschulz92]

Maybe the issue is not even in this repo but in the actual VS Code plugin? I may try to explain the issue again.

In my project I started to use the "share steps" feature more often. I have a shared folder with many steps. If you have a Python file without a class, the steps are found perfectly fine. If I have a Python class with a class, so we must have the instance to map the method parameters, the issue comes up. VS code seems to handle paths differently compared to Git for Windows. The issue is visible in VS code only.

# Inject instance in each class method (hook/step)
def update_step_registry_with_class(instance, file_path):
    # Resolve the absolute path from relative path
    # Note: relative path syntax ".." can appear in between the file_path too like "<Project_Root>/../../Other_Project/src/step_impl/file.py"
    file_path = os.path.abspath(file_path) if ".." in str(file_path) else file_path
    method_list = registry.get_all_methods_in(file_path)
    for info in method_list:
        class_methods = [x[0] for x in inspect.getmembers(instance, inspect.ismethod)]
        if info.impl.__name__ in class_methods:
            info.instance = instance
    return method_list

If the paths are different, the instance is not set properly. Therefore I tried to find a solution so that the paths are aligned. It should not matter if the C driver is written in capital or lower case. But in my case, the var method_list is empty.

[sschulz92]

@zabil @chadlwilson I've added two tests to cover the two different cases. Does the issue makes more sense to you now? 😄

[chadlwilson]

It may be the right thing to do, but these operations probably require filesystem operations (since not using PurePath, and no way to normalize symlinks/junction points without FS access).

This will likely make things slower. If we can achieve the goal correctly & securely, without resolving actual file system paths we should try to do that.

Also, do we need to mix os.path and pathlib usage?

1. are you intentionally trying to add normalization of .. and symlinks via resolve()?
2. if not, why not just use normcase for all platforms? Docs say it's a no-op on case-sensitive filesystems, so it'd be cleaner to use the library to avoid platform specific logic.

[chadlwilson]

There is already some sort of path normalisation logic at both

gauge-python/getgauge/impl_loader.py

Line 78 in 5ad2747

rel_path = os.path.normpath(file_path.replace(base_dir + os.path.sep, ''))

and

gauge-python/getgauge/impl_loader.py

Line 102 in 5ad2747

file_path = os.path.abspath(file_path) if ".." in str(file_path) else file_path

(...and other places)

This is getting rather confusing to me. It feels like we should have one place/consistent approach to how/when we normalise paths, as there are already a lot of hacks around the place :-(

Do you know what the origin of the inconsistent C:\blah vs c:\blah is? Something inconsistent in VS Code? or the gauge VSCode plugin?

[sschulz92]

This is getting rather confusing to me. It feels like we should have one place/consistent approach to how/when we normalise paths, as there are already a lot of hacks around the place :-(

Do you know what the origin of the inconsistent C:\blah vs c:\blah is? Something inconsistent in VS Code? or the gauge VSCode plugin?

Totally agree, it is very difficult already. From my perspective it does not need to be that difficult as we want to "simply" load steps from different file locations.

Regarding your question: no, I am not aware of the origin. I started to use this feature recently but I guess the issue might exist even longer (no prove for that!). Since the plugin is written in TypeScript I cannot help much as I have too less knowledge about VS code and its behavior.

[chadlwilson]

Functional test failure is my bad; will sort it out.

Should be fixed now if you merge or rebase from master.

[sschulz92]

Functional test failure is my bad; will sort it out.

Should be fixed now if you merge or rebase from master.

You are soo fast in responding.. wow 😄

[chadlwilson]

You are soo fast in responding.. wow 😄

Right now I'm not working, so have a bit more time to spend on OSS (although Gauge is not my focus) and I've been shepherding some projects through Gradle upgrades (including Gauge functional tests), which is why this broke, and also why I should fix it promptly :)

The root problem here is that the gauge func/integration and LSP tests aren't modelled as reusable GitHub actions, so the requirements to run them are not self contained in their own repo and are thus distributed around all the plugin projects which need changing individually for certain changes.

[sschulz92]

@chadlwilson the failed check passed on the forked branch, so not sure about that.

In general, what I did:

1. I am resolving the impl_dir once, so that I can import steps from outside the project_root too (folder must not start with the same folder, e.g. step_impl)
2. I am using glob to easier get all Python files with absolute paths without any other call (e.g. normpath) required
3. Since update_step_registry_with_class is called in impl_loader only, I am sure that the paths will be the same all the time, so no normpath required anymore
4. I changed the related tests to use the root method load_impls() to test the same thing nevertheless
5. I moved the folder test_relative_imports into the tests directory where it belongs too (in my mind)

That's mainly it, what do you think about it? I added a few debug logs, not sure if we really want them. While debugging they were quite helpfull to see the differences between current released version and my snapshot one. On my machine, this version of the Python runner works like a charm.