-
Notifications
You must be signed in to change notification settings - Fork 17
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 cli tests in windows #774
Conversation
054087a
to
46be2c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! I had some questions, and some stylistic suggestions but you're free to take them or leave them.
The only thing I'm particularly (not too much though) concerned about is the prettier change. I'd rather we don't see crlf in the repository one day ;)
If the tests are fixed, can we change travis configuration to run the full CLI test suite on Windows now?
packages/cli/tests/unit/record/action/configureHostAndPort.spec.ts
Outdated
Show resolved
Hide resolved
c142efa
to
3dad381
Compare
Thanks for the excellent review! I addressed all of your comments. Let me know if you have any other questions or concerns. I made it so all of the tests except for one run in Windows on Travis. |
@@ -0,0 +1,17 @@ | |||
const FILTERED_TESTS = process.platform === 'win32' ? ['fingerprintWatchCommand'] : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this test need to be filtered? If there's something left to fix for this issue, that's fine, but please remember to create another one to keep track of the loose end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one gives this error in Travis on Windows:
You can see it in the logs on this build: https://app.travis-ci.com/github/getappmap/appmap-js/jobs/585469980#L220
I'll make a new issue for it.
Thanks, that looks much better! Now I'm only worried about those line endings. Perhaps it would be a good idea to add a
— haven't checked if it actually works, YMMV. Another option would be to do |
15ca190
to
28636c6
Compare
I put all of the tests into the platform-specific The strange thing is that this test passes for me locally on Windows and on Linux, so I'm not sure how to debug this. I tried changing the Do you have any thoughts? |
67e3f3e
to
95a03da
Compare
Okay, I got it to work. Turns out that forcing
was causing issues on Travis. I'm not sure why. But I needed to force Let me know if there's anything else you think needs to be changed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now, thanks for figuring out this pesky EOL business!
(Please remember to rebase and fixup everything before merging.)
c626172
to
2e9d82e
Compare
2e9d82e
to
cf2baee
Compare
🎉 This PR is included in version @appland/appmap-v3.45.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Fixes getappmap/board#188