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

Move slash replacement to the end #483

Closed
wants to merge 2 commits into from
Closed

Conversation

cemremengu
Copy link
Contributor

Make sure the slash replacement is done

Make sure the slash replacement is done
@cemremengu
Copy link
Contributor Author

cemremengu commented Dec 5, 2018

Just tested this manually from my node_modules and it fixes the problem. I was going to add a test case but I think it is best to enable windows testing on travis for these kind of things once it is ready.

I would buy you guys a coffee if you can patch this soon :)

lib/test.js Outdated
' ' + (this.name || '').trim()
: path.basename(main)) + ' ' + (this.name || '').trim()

return name.replace(/\\/g, '/')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this be an issue if you want to have a slash in the name?

For example: t.test('make sure \\ is replaced with /', t => { ... }) would result in some confusing output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes you are right. Not sure why the current one doesnt work even though the replacement. Whats a more suitable place to handle this? Any hints are appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm guessing that maybe the name is getting set somewhere in the Spawn subclass perhaps? I'd been using the Windows support on Travis briefly, but ran into too many issues around signal and subprocess differences, so I think the only way forward is for me (or someone) to spin up a Windows VM and get all the tests passing locally first. The turnaround is just too slow to do with a remote CI system otherwise, and a known-failing test is worse than no test at all.

@cemremengu
Copy link
Contributor Author

cemremengu commented Dec 5, 2018

I am confused actually. The version I have (^12.0.0) has a different fullname method than the master. The version in master works fine actually. Maybe I should have more coffee not sure what's going on 😄

Not sure why semver is not bumping to next minor version which this was fixed

@isaacs
Copy link
Member

isaacs commented Dec 5, 2018

Oh! Looks like I never published the code on master branch, because I was still fantasizing that I'd get everything passing on Windows first. Maybe I should just bite the bullet and accept it as-is :)

@cemremengu
Copy link
Contributor Author

Since I use windows, I can try getting tests to run locally first. Not sure how tough it will be for me though, this project looks beyond my skills but I am up for the challange :)

@cemremengu cemremengu closed this Dec 8, 2018
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.

None yet

2 participants