Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for loading features from the filesystem #33

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dazwin
Copy link

@dazwin dazwin commented Nov 13, 2019

This is the first option to resolve #32

@lsuski
Copy link
Contributor

lsuski commented Nov 13, 2019

This one is better IMHO. It's simpler. Assets are used by default but if scheme is specified then it can be also resolved. In future it could also load features from external server with https uri. I'll try to check it out and release new version in this week

@lsuski
Copy link
Contributor

lsuski commented Nov 13, 2019

@dazwin could you add android instrumented test for this case and run it on api level 21, 23, 27, 29? Reading file from device require storage permission and it is not directly allowed with scheme file on newer apis

@dazwin
Copy link
Author

dazwin commented Nov 13, 2019

I've added 3 unit tests to cover the new code that's been added, using the same approach as existing tests - i.e. to mock resource loading rather than read from actual resources.

Because I'm not directly accessing the filesystem either, any tests that relate specifically to the filesystem belong in cucumber-java (to test Multiloader and it's related classes).

There are a couple of fixes - file-based URI's don't require a double-slash, so the test is now just file:/. I'm also now using the class loader provided by the context, not my own thread (probably the same, but cleaner).

I've also added a note to the README.

FWIW, the other Multiloader scheme support will probably work as well (classpath:) but I have no need for this and have not done any testing on it.

@dazwin
Copy link
Author

dazwin commented Nov 13, 2019

This PR does not load assets just by including a file in the URI, btw. The Cucumber framework is already injecting 'file' as the scheme if none is specified and AndroidResourceLoader sees this. If you debug your master branch you'll see that you actually get 'file:features' (but android resources are loaded nevertheless). This is why #34, although more correct, would mean a breaking change.

This PR is subtle. As mentioned, it detects an absolute URI as opposed to relative. "file:features" will load Android resources (as before), but "file:/features" will attempt to look at the filesystem.

@lsuski
Copy link
Contributor

lsuski commented Nov 13, 2019

I agree but there is an issue which simple unit test executed on jvm does not cover. Although MultiLoader is tested in cucumber-java it is not tested in Android environment. Cucumber-android runs on real device so we need to have test which executes on Android device. Afaik trying to read file from uri file:// in Android filesystem, e.g. sdcard will throw an error

@dazwin
Copy link
Author

dazwin commented Nov 14, 2019

Testing a filesystem is far from simple. There are a multitude of problems which may cause a file read to fail and permissions requested via AndroidManifest is just one of them.

Reading a file does not require any particular permission by itself, though it certainly depends on your particular environment and where you're trying to read the file from. For my use case, I'm not trying to read from an SD Card and I'm running in conditions that do not need READ_EXTERNAL_STORAGE, so any such testing would be a false negative.

@lsuski
Copy link
Contributor

lsuski commented Nov 14, 2019

So from where do you want to read file? Your device/emulator has custom Android with different file access managment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running features not found in resources
2 participants