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

🐛 handle incident uri on windows #338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

djzager
Copy link
Member

@djzager djzager commented Feb 3, 2025

Fixes #252

@djzager djzager marked this pull request as ready for review February 3, 2025 15:59
@djzager djzager requested a review from a team as a code owner February 3, 2025 15:59
@djzager
Copy link
Member Author

djzager commented Feb 3, 2025

@abrugaro I believe what is happening is that the path-browserify doesn't attempt to handle windows paths at all so our regex breaks on paths with \ path separators. This SHOULD fix the problem but I'd like for you to build it (or I think you should be able to grab a vsix off the build action) and give it a go if you could please.

ibolton336
ibolton336 previously approved these changes Feb 3, 2025
Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Feels like the basic function that does "take the workspace root Uri and the file Uri and produce a file path that is the relative path from workspace root to the file" needs to be moved to a file under webview-ui/src/utils so we can actually setup some unit tests on it to make sure it works under different permutations.

@djzager djzager force-pushed the incidents-on-windows branch from c6b0358 to 5e4f353 Compare February 3, 2025 21:14
@djzager djzager requested review from sjd78 and ibolton336 February 3, 2025 21:15
sjd78 added a commit to sjd78/editor-extensions that referenced this pull request Feb 4, 2025
Enable mocha tests on `webview-ui` by:
  - moving mocha deps to root `package.json`
  - adding `.mocharc.json` to each project that has unit tests
  - adopt `tsx` instead of `ts-node`

Note: A glob is in place on `webview-ui`'s test script since
no unit test currently exist to run.  In that case, it isn't
really an error.  In the future, once a test is added, the glob
check should be removed so mocha will fail if tests are not found.

Supports: konveyor#338

Signed-off-by: Scott J Dickerson <[email protected]>
sjd78 added a commit to sjd78/editor-extensions that referenced this pull request Feb 4, 2025
Enable mocha tests on `webview-ui` by:
  - moving mocha deps to root `package.json`
  - adding `.mocharc.json` to each project that has unit tests
  - adopt `tsx` instead of `ts-node`

Note: A glob is in place on `webview-ui`'s test script since
no unit test currently exist to run.  In that case, it isn't
really an error.  In the future, once a test is added, the glob
check should be removed so mocha will fail if tests are not found.

Supports: konveyor#338

Signed-off-by: Scott J Dickerson <[email protected]>
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

I posted #343 to enable tests to run with npm run test. This happens at CI time. Once that PR gets in, this unit test can run!

This is a good unit test to have.

@abrugaro
Copy link

abrugaro commented Feb 4, 2025

@djzager I grabbed the vsix from here and the bug is still reproducible.

sjd78 added a commit that referenced this pull request Feb 4, 2025
Enable mocha tests on `webview-ui` by:
  - moving mocha deps to root `package.json`
  - adding `.mocharc.json` to each project that has unit tests
  - adopt `tsx` instead of `ts-node`

Note: A glob is in place on `webview-ui`'s test script since no unit
test currently exist to run. In that case, it isn't really an error. In
the future, once a test is added, the glob check should be removed so
mocha will fail if tests are not found.

Supports: #338

Signed-off-by: Scott J Dickerson <[email protected]>
@djzager djzager force-pushed the incidents-on-windows branch from c7f7270 to c2b0bdb Compare February 4, 2025 17:08
@sjd78
Copy link
Member

sjd78 commented Feb 4, 2025

Ugh so my fancy glob command on the test script also lets things pass when the tests actually fail:
https://github.com/konveyor/editor-extensions/actions/runs/13140923549/job/36667860122?pr=338#step:5:195

Feel free to change the webview-ui/package.json value "scripts.test" value to just "mocha".

@djzager
Copy link
Member Author

djzager commented Feb 4, 2025

Ugh so my fancy glob command on the test script also lets things pass when the tests actually fail: https://github.com/konveyor/editor-extensions/actions/runs/13140923549/job/36667860122?pr=338#step:5:195

Feel free to change the webview-ui/package.json value "scripts.test" value to just "mocha".

Thank you for noticing that. I was said that the tests didn't fail 😁

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.

The "folder" column in the incidents always displays "file://"
4 participants