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 require path in test #1122

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Conversation

Legogris
Copy link
Contributor

npm run dev fails on current master due to invalid casing in filename. This fixes that.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.517% when pulling 9be7b90 on Legogris:fix/requiretestpath into 12b2d2b on gregnb:master.

@gabrielliwerant
Copy link
Collaborator

Hm, I don't experience this issue. Can you provide details of your setup like version for npm, node, operating system, etc., so that I might gain some insight?

@gabrielliwerant gabrielliwerant added the needs verification For issues that can't be reproduced or are otherwise unclear label Dec 13, 2019
@Legogris
Copy link
Contributor Author

I don't suppose you're on Windows @gregnb (paths are case-insensitive on Windows but not on Linux/OS X)? There is no Cities.json or Cities.js in the folder.

@patorjk
Copy link
Collaborator

patorjk commented Dec 14, 2019

I can verify this, it doesn't work on codesandbox: https://codesandbox.io/s/github/gregnb/mui-datatables

I also added a fix as part of this PR: #1087

Edit: Actually, I just tried again, and it currently works fine on Mac OS X (which is what I use). It was only on codesandbox where I noticed the issue (I was getting ready to put in the PR and include a link to codesandbox and it didn't work so I fixed the typo before submitting).

@gabrielliwerant
Copy link
Collaborator

I don't see the problem on Windows, but I don't see any harm in changing it to lowercase, as that is how it is used elsewhere, and that is the exact filename, so it should be fixed. Thanks for pointing this out @Legogris and thanks for helping to verify @patorjk.

@gabrielliwerant gabrielliwerant added bug enhancement lgtm and removed needs verification For issues that can't be reproduced or are otherwise unclear labels Dec 14, 2019
@Legogris
Copy link
Contributor Author

Legogris commented Dec 15, 2019

I don't see the problem on Windows, but I don't see any harm in changing it to lowercase, as that is how it is used elsewhere, and that is the exact filename, so it should be fixed. Thanks for pointing this out @Legogris and thanks for helping to verify @patorjk.

FYI for the future, apart from Windows on NTFS/FAT, paths are generally case-sensitive on all filesystems and OS's in mainstream use today.

It's expected that Windows doesn't care about casing; Linux/BSD/OS X does.

@gabrielliwerant
Copy link
Collaborator

Closes #1157.

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

Successfully merging this pull request may close these issues.

4 participants