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

Fix tests on windows for using illuminate/filesystem and add option for absolute paths #176

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

Conversation

florisbosch
Copy link
Contributor

Related: #174

This pull request fixes tests on windows for using illuminate/filesystem and adds an option for absolute paths.

The fix for windows was to check for existing files before trying to use the filesize or lastmodified functions.

I've added the option to enable/disable absolute paths. It is working on my machine but I don't think this is the most optimal or elegant way to solve this problem. With this solution the current features are backwards compatible without any changes in current implementations.

My idea for handling this problem in the future is to allow for the combination of multiple disks, this way we can use multiple local or cloud folders in the same viewer. Using absolute paths is usually not the most secure way of handling files especially when you are not fully in control of the hosting of your Laravel application.

@florisbosch
Copy link
Contributor Author

It seems like something is still off with reading the files.
I think I missed something with merging my local files somehow, will check it tonight

@florisbosch florisbosch force-pushed the main branch 4 times, most recently from fccce3a to 65be109 Compare December 19, 2022 22:59
@florisbosch florisbosch force-pushed the main branch 2 times, most recently from bc482f5 to 4a74b52 Compare December 19, 2022 23:23
@florisbosch
Copy link
Contributor Author

Hi @arukompas,

I can't really debug this problem because I cannot run the windows containers on my Macbook at the moment.
I am on holiday for the next 2 weeks hopefully I can manage to fix this last problem after that.

Sorry for the long wait on this issue!

@arukompas
Copy link
Contributor

Hey @florisbosch , thank you so much for the effort and please don't fret it and enjoy your holiday :)

I'm off as well, and if I need to tag another release/fix, I can just revert the previous merge and we can re-apply it later.

Enjoy your holidays! 🎉

@florisbosch
Copy link
Contributor Author

florisbosch commented Dec 22, 2022

Hi @arukompas,

I managed to setup a VM with windows and fixed a few test.
There only seems to be a problem with the random order of tests at this moment. Individually all tests pass but when executed in random order they fail, but not always probably depends on which order.

Do you have any idea what can cause this problem?

image

@florisbosch florisbosch marked this pull request as ready for review December 22, 2022 10:17
@florisbosch
Copy link
Contributor Author

florisbosch commented Jan 24, 2023

Hi @arukompas,
Did you have any chance to look into the order of the tests?

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.

2 participants