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: for filepaths/folders templates, the icon type of the suggestion… #207

Merged
merged 2 commits into from
Mar 23, 2024

Conversation

alanhe421
Copy link
Contributor

@alanhe421 alanhe421 commented Mar 12, 2024

For filepaths / folder templates, the icon type of the suggestion needs to be specified to avoid going to the default icon.

thanks for the review.
@cpendery

image

Copy link
Member

@cpendery cpendery left a comment

Choose a reason for hiding this comment

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

Can you add a test that covers this case, something like ls sr and just take the first item so we know it has a folder icon and then do something like ls READ to capture that it has the file icon?

Trying to have all PRs adding tests for the behavior they fix moving forward

@alanhe421 alanhe421 force-pushed the fix/file-template branch 2 times, most recently from 111b2df to d7aabfc Compare March 17, 2024 04:21
@alanhe421
Copy link
Contributor Author

alanhe421 commented Mar 17, 2024

Can you add a test that covers this case, something like ls sr and just take the first item so we know it has a folder icon and then do something like ls READ to capture that it has the file icon?

Trying to have all PRs adding tests for the behavior they fix moving forward

@cpendery

Thank you very much for the suggestion, I've added the corresponding test case.

image

image

I verified that it was already OK. But I found a problem with the other test cases.

Do you want to fix it immediately or skip it first?

image

@cpendery
Copy link
Member

Can you add a test that covers this case, something like ls sr and just take the first item so we know it has a folder icon and then do something like ls READ to capture that it has the file icon?
Trying to have all PRs adding tests for the behavior they fix moving forward

@cpendery

Thank you very much for the suggestion, I've added the corresponding test case.

image

image

I verified that it was already OK. But I found a problem with the other test cases.

Do you want to fix it immediately or skip it first?

image

If it continues to be an issue, feel free to skip it. I'll address it later

@cpendery
Copy link
Member

@alanhg Looks like you'll need to update the e2e tests too since they also run into those icon issues

@alanhe421
Copy link
Contributor Author

alanhe421 commented Mar 21, 2024

@cpendery
Hi. I realized that e2e tests don't check icons or anything like that, this PR just changed the icon of the question, the e2e interaction test itself doesn't need to change, right? Or do you want to add the corresponding test case, but we have already added the check in the unit test.

Also, I found a lot of failures in e2e tests.

image

@cpendery
Copy link
Member

@cpendery Hi. I realized that e2e tests don't check icons or anything like that, this PR just changed the icon of the question, the e2e interaction test itself doesn't need to change, right? Or do you want to add the corresponding test case, but we have already added the check in the unit test.

Also, I found a lot of failures in e2e tests.

image

The test generator results lead suggestions in src\tests\ui\autocomplete.test.ts does use the icon of ls which needs updated. That's the only test that's failing in the jobs. The e2e framework is definitely still a work in progress tui-test. If you could open any issues there, that would be great.

…n needs to be specified to avoid going to the default icon.
@alanhe421
Copy link
Contributor Author

@cpendery Hi. I realized that e2e tests don't check icons or anything like that, this PR just changed the icon of the question, the e2e interaction test itself doesn't need to change, right? Or do you want to add the corresponding test case, but we have already added the check in the unit test.
Also, I found a lot of failures in e2e tests.
image

The test generator results lead suggestions in src\tests\ui\autocomplete.test.ts does use the icon of ls which needs updated. That's the only test that's failing in the jobs. The e2e framework is definitely still a work in progress tui-test. If you could open any issues there, that would be great.

I have updated the test, thank you.

image

@cpendery cpendery merged commit 34049e9 into microsoft:main Mar 23, 2024
10 checks passed
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